Обсуждение: Handling dropped attributes in pglogical_proto

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

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

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

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