Handling dropped attributes in pglogical_proto

Started by Konstantin Knizhnikover 9 years ago4 messages
#1Konstantin Knizhnik
k.knizhnik@postgrespro.ru

Hi,

pglogical_read_tuple from pglogical_proto.c contains the following code:

natts = pq_getmsgint(in, 2);
if (rel->natts != natts)
elog(ERROR, "tuple natts mismatch, %u vs %u", rel->natts, natts);

But if table was just altered and some attribute was removed from the
table, then rel->natts can be greater than natts.
The problem can be fixed by the following patch:

--- a/pglogical_proto.c
+++ b/pglogical_proto.c
@@ -263,10 +263,15 @@ pglogical_read_tuple(StringInfo in, PGLogicalRelation *rel,
         {
                 int                     attid = rel->attmap[i];
                 Form_pg_attribute att = desc->attrs[attid];
-               char            kind = pq_getmsgbyte(in);
+               char            kind;
                 const char *data;
                 int                     len;
+               if (att->atttypid == InvalidOid) {
+                       continue;
+               }
+               kind = pq_getmsgbyte(in);
+
                 switch (kind)
                 {
                         case 'n': /* null */

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Michael Paquier
michael.paquier@gmail.com
In reply to: Konstantin Knizhnik (#1)
Re: Handling dropped attributes in pglogical_proto

On Wed, Sep 28, 2016 at 11:25 PM, Konstantin Knizhnik
<k.knizhnik@postgrespro.ru> wrote:

But if table was just altered and some attribute was removed from the table,
then rel->natts can be greater than natts.

This is part of pglogical, so you may want to reply on the dedicated
thread or send directly a patch to them. By the way, this code path
may need to care as well about attisdropped. It is never good not to
check for it.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Petr Jelinek
petr@2ndquadrant.com
In reply to: Michael Paquier (#2)
Re: Handling dropped attributes in pglogical_proto

On 29/09/16 05:33, Michael Paquier wrote:

On Wed, Sep 28, 2016 at 11:25 PM, Konstantin Knizhnik
<k.knizhnik@postgrespro.ru> wrote:

But if table was just altered and some attribute was removed from the table,
then rel->natts can be greater than natts.

This is part of pglogical, so you may want to reply on the dedicated
thread or send directly a patch to them. By the way, this code path
may need to care as well about attisdropped. It is never good not to
check for it.

Agreed this does not belong to hackers and should reported on github.

But just as note the rel variable is not Relation, it's
PGLogicalRelation which gets populated by relation message from the
upstream so if you observe the behavior mentioned in the original email,
you are probably doing something unexpected there (Konstantin is not
using vanilla pglogical). Also the attmap should never contain
attisdropped attributes.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Craig Ringer
craig@2ndquadrant.com
In reply to: Petr Jelinek (#3)
Re: Handling dropped attributes in pglogical_proto

On 29 September 2016 at 12:53, Petr Jelinek <petr@2ndquadrant.com> wrote:

On 29/09/16 05:33, Michael Paquier wrote:

On Wed, Sep 28, 2016 at 11:25 PM, Konstantin Knizhnik
<k.knizhnik@postgrespro.ru> wrote:

But if table was just altered and some attribute was removed from the table,
then rel->natts can be greater than natts.

This is part of pglogical, so you may want to reply on the dedicated
thread or send directly a patch to them. By the way, this code path
may need to care as well about attisdropped. It is never good not to
check for it.

Agreed this does not belong to hackers and should reported on github.

But just as note the rel variable is not Relation, it's
PGLogicalRelation which gets populated by relation message from the
upstream so if you observe the behavior mentioned in the original email,
you are probably doing something unexpected there (Konstantin is not
using vanilla pglogical). Also the attmap should never contain
attisdropped attributes.

Yeah, and if you're relying on attno equivalence you're also going to
have to deal with it in dump/restore unless you only create new nodes
using physical copy. (See bdr's bdr_dump).

BDR relies on attno equivalence. pglogical avoids doing because of the
pain it caused in BDR.

On a side note, do NOT report bugs on ANY software when you're talking
about a modified version unless you clearly disclose that it's
modified. You'll waste everyone's time. We already spent a while
looking into a logical decoding issue you reported that turned out to
be caused by your team's patches to add 2PC decoding support, which
you didn't mention in the report. If you want anyone to listen to your
reports you really need to be very explicit about whether it's a
modified version or not. If it's modified, make sure you can reproduce
the problem in the unmodified version or explain the situation more.

If you want to ask things about BDR and pglogical, now that it's clear
that in-core logical replication is going in another direction so
-hackers isn't the place for it, please use bdr-list@2ndquadrant.com .

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers