On Monday, October 28, 2024 1:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Oct 28, 2024 at 7:43 AM Peter Smith <smithpb2250@gmail.com>
> wrote:
>
> >
> > 4. pgoutput_column_list_init
> >
> > - if (att->attisdropped || att->attgenerated)
> > + if (att->attisdropped)
> > continue;
> >
> > + if (att->attgenerated)
> > + {
> > + if (bms_is_member(att->attnum, cols)) gencolpresent = true;
> > +
> > + continue;
> > + }
> > +
> > nliveatts++;
> > }
> >
> > /*
> > - * If column list includes all the columns of the table,
> > - * set it to NULL.
> > + * If column list includes all the columns of the table
> > + * and there are no generated columns, set it to NULL.
> > */
> > - if (bms_num_members(cols) == nliveatts)
> > + if (bms_num_members(cols) == nliveatts && !gencolpresent)
> > {
> >
> > Something seems not quite right (or maybe redundant) with this logic.
> > For example, because you unconditionally 'continue' for generated
> > columns, then AFAICT it is just not possible for bms_num_members(cols)
> > == nliveatts and at the same time 'gencolpresent' to be true. So you
> > could've just Asserted (!gencolpresent) instead of checking it in the
> > condition and mentioning it in the comment.
I think it's possible for the condition you mentioned to happen.
For example:
CREATE TABLE test_mix_4 (a int primary key, b int, d int GENERATED ALWAYS AS (a + 1) STORED);
CREATE PUBLICATION pub FOR TABLE test_mix_4(a, d);
> >
>
> It seems part of the logic is redundant. I propose to change something along the
> lines of the attached. I haven't tested the attached change as it shows how we
> can improve this part of code.
Thanks for the changes. I tried and faced an unexpected behavior
that the walsender will report Error "cannot use different column lists fo.."
in the following case:
Pub:
CREATE TABLE test_mix_4 (a int PRIMARY KEY, b int, c int, d int GENERATED ALWAYS AS (a + 1) STORED);
ALTER TABLE test_mix_4 DROP COLUMN c;
CREATE PUBLICATION pub_mix_7 FOR TABLE test_mix_4 (a, b);
CREATE PUBLICATION pub_mix_8 FOR TABLE test_mix_4;
Sub:
CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub_mix_7, pub_mix_8;
The pub_mix_7 publishes column a,b which should be converted to NULL
in pgoutput, but was not due to the check of att_gen_present.
Based on above, I feel we can keep the original code as it is.
Best Regards,
Hou zj
