Re: Pgoutput not capturing the generated columns

On Thu, 24 Oct 2024 at 12:17, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Oct 23, 2024 at 11:51 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Thanks. One more thing that I didn't like about the patch is that it
> > used column_list to address the "publish_generated_columns = false"
> > case such that we build column_list without generated columns for the
> > same. The first problem is that it will add overhead to always probe
> > column_list during proto.c calls (for example during
> > logicalrep_write_attrs()), then it makes the column_list code complex
> > especially the handling in pgoutput_column_list_init(), and finally
> > this appears to be a misuse of column_list.
> >
> > So, I suggest remembering this information in RelationSyncEntry and
> > then using it at the required places. We discussed above that
> > contradictory values of "publish_generated_columns" across
> > publications for the same relations are not accepted, so we can detect
> > that during get_rel_sync_entry() and give an ERROR for the same.
> >
>
> The changes in tablesync look complicated and I am not sure whether it
> handles the conflicting publish_generated_columns option correctly. I
> have few thoughts for the same.
> * The handling of contradictory options in multiple publications needs
> to be the same as for column lists. I think it is handled (a) during
> subscription creation, (b) during copy in fetch_remote_table_info(),
> and (c) during replication. See Peter's email
> (https://www.postgresql.org/message-id/CAHut%2BPs985rc95cB2x5yMF56p6Lf192AmCJOpAtK_%2BC5YGUF2A%40mail.gmail.com)
> to understand why this new option has to be handled in the same way as
> the column list.
>
> * While fetching column list via pg_get_publication_tables(), we
> should detect contradictory publish_generated_columns options similar
> to column lists, and then after we get publish_generated_columns as
> return value, we can even use that while fetching attribute
> information.

Modified it to detect conflicting column lists for publications,
allowing the detection of publish_generated_columns conflicts using
the same code currently used for column list detection.

> A few additional comments:
> 1.
> - /* Regular table with no row filter */
> - if (lrel.relkind == RELKIND_RELATION && qual == NIL)
> + /*
> + * Check if the remote table has any generated columns that should be
> + * copied.
> + */
> + for (int i = 0; i < relmapentry->remoterel.natts; i++)
> + {
> + if (lrel.attremotegen[i])
> + {
> + gencol_copy_needed = true;
> + break;
> + }
> + }
>
> Can't we get this information from fetch_remote_table_info() instead
> of traversing the entire column list again?

Yes, modified

> 2.
> @@ -1015,7 +1110,6 @@ fetch_remote_table_info(char *nspname, char *relname,
>   ExecDropSingleTupleTableSlot(slot);
>
>   lrel->natts = natt;
> -
>   walrcv_clear_result(res);
>
> Spurious line removal.

Fixed this

> 3. Why do we have to specifically exclude generated columns of a
> subscriber-side table in make_copy_attnamelist()? Can't we rely on
> fetch_remote_table_info() and logicalrep_rel_open() that the final
> remote attrlist will contain the generated column only if the
> subscriber doesn't have a generated column otherwise it would have
> given an error in logicalrep_rel_open()?

Yes, it can be used. The updated v42 version of patch has the changes
for the same.

Regards,
Vignesh