Re: Pgoutput not capturing the generated columns

On Wed, 26 Jun 2024 at 08:06, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok. Here are my review comments for patch v10-0003
>
> ======
> General.
>
> 1.
> The patch has lots of conditions like:
> if (att->attgenerated && (att->attgenerated !=
> ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
> continue;
>
> IMO these are hard to read. Although more verbose, please consider if
> all those (for the sake of readability) would be better re-written
> like below :
>
> if (att->generated)
> {
> if (!include_generated_columns)
> continue;
>
> if (att->attgenerated != ATTRIBUTE_GENERATED_STORED)
> continue;
> }
Fixed

> ======
> contrib/test_decoding/test_decoding.c
>
> tuple_to_stringinfo:
>
> NITPICK = refactored the code and comments a bit here to make it easier
> NITPICK - there is no need to mention "virtual". Instead, say we only
> support STORED
Fixed

> ======
> src/backend/catalog/pg_publication.c
>
> publication_translate_columns:
>
> NITPICK - introduced variable 'att' to simplify this code
Fixed

> ~
>
> 2.
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
> + errmsg("cannot use virtual generated column \"%s\" in publication
> column list",
> + colname));
>
> Is it better to avoid referring to "virtual" at all? Instead, consider
> rearranging the wording to say something like "generated column \"%s\"
> is not STORED so cannot be used in a publication column list"
Fixed

> ~~~
>
> pg_get_publication_tables:
>
> NITPICK - split the condition code for readability
Fixed

> ======
> src/backend/replication/logical/relation.c
>
> 3. logicalrep_rel_open
>
> + if (attr->attgenerated && attr->attgenerated != ATTRIBUTE_GENERATED_STORED)
> + continue;
> +
>
> Isn't this missing some code to say "entry->attrmap->attnums[i] =
> -1;", same as all the other nearby code is doing?
Fixed

> ~~~
>
> 4.
> I felt all the "generated column" logic should be kept together, so
> this new condition should be combined with the other generated column
> condition, like:
>
> if (attr->attgenerated)
> {
> /* comment... */
> if (!MySubscription->includegencols)
> {
> entry->attrmap->attnums[i] = -1;
> continue;
> }
>
> /* comment... */
> if (attr->attgenerated != ATTRIBUTE_GENERATED_STORED)
> {
> entry->attrmap->attnums[i] = -1;
> continue;
> }
> }
Fixed

> ======
> src/backend/replication/logical/tablesync.c
>
> 5.
> + if (gencols_allowed)
> + {
> + /* Replication of generated cols is supported, but not VIRTUAL cols. */
> + appendStringInfo(&cmd, " AND a.attgenerated != 'v'");
> + }
>
> Is it better here to use the ATTRIBUTE_GENERATED_VIRTUAL macro instead
> of the hardwired 'v'? (Maybe add another TODO comment to revisit
> this).
>
> Alternatively, consider if it is more future-proof to rearrange so it
> just says what *is* supported instead of what *isn't* supported:
> e.g. "AND a.attgenerated IN ('', 's')"
I feel we should use ATTRIBUTE_GENERATED_VIRTUAL macro. Added a TODO.

> ======
> src/test/subscription/t/011_generated.pl
>
> NITPICK - some comments are missing the word "stored"
> NITPICK - sometimes "col" should be plural "cols"
> NITPICK = typo "GNERATED"
Add the relevant changes.

> ======
>
> 6.
> In a previous review [1, comment #3] I mentioned that there should be
> some docs updates on the "Logical Replication Message Formats" section
> 53.9. So, I expected patch 0001 would make some changes and then patch
> 0003 would have to update it again to say something about "STORED".
> But all that is missing from the v10* patches.
>
> ======
Will fix in upcoming version

>
> 99.
> See also my nitpicks diff which is a top-up patch addressing all the
> nitpick comments mentioned above. Please apply all of these that you
> agree with.
Applied Relevant changes

Please refer v14 patch for the changes [1].

[1]: https://www.postgresql.org/message-id/CANhcyEW95M_usF1OJDudeejs0L0%2BYOEb%3DdXmt_4Hs-70%3DCXa-g%40mail.gmail.com

Thanks and Regards,
Shlok Kyal