On Thu, 24 Oct 2024 at 16:44, Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Thu, Oct 24, 2024 at 12:15 PM vignesh C <vignes...@gmail.com> wrote:
> >
> > The attached v41 version patch has the changes for the same.
> >
>
> Please find comments for the new version as follows:
> 1.
> + Generated columns may be skipped during logical replication
> according to the
> + <command>CREATE PUBLICATION</command> option
> + <link
> linkend="sql-createpublication-params-with-publish-generated-columns">
> + <literal>include_generated_columns</literal></link>.
>
> The above statement doesn't sound to be clear. Can we change it to:
> "Generated columns are allowed to be replicated during logical
> replication according to the <command>CREATE PUBLICATION</command>
> option .."?
Modified
> 2.
> static void publication_invalidation_cb(Datum arg, int cacheid,
> uint32 hashvalue);
> -static void send_relation_and_attrs(Relation relation, TransactionId xid,
> - LogicalDecodingContext *ctx,
> - Bitmapset *columns);
> static void send_repl_origin(LogicalDecodingContext *ctx,
> ...
> ...
> static RelationSyncEntry *get_rel_sync_entry(PGOutputData *data,
> Relation relation);
> +static void send_relation_and_attrs(Relation relation, TransactionId xid,
> + LogicalDecodingContext *ctx,
> + RelationSyncEntry *relentry);
>
> Why the declaration of this function is changed?
Two changes were made: a) The function declaration need to be moved
down as the RelationSyncEntry structure is defined below. b) Bitmapset
was replaced with RelationSyncEntry to give send_relation_and_attrs
access to RelationSyncEntry.pubgencols and RelationSyncEntry.columns.
Instead of adding a new parameter to the function, RelationSyncEntry
was utilized, as it contains both pubgencols and columns members.
> 3.
> + /*
> + * Skip publishing generated columns if the option is not specified or
> + * if they are not included in the column list.
> + */
> + if (att->attgenerated && !relentry->pubgencols && !columns)
>
> In the comment above, shouldn't "specified or" be "specified and"?
Modified
> 4.
> +pgoutput_pubgencol_init(PGOutputData *data, List *publications,
> + RelationSyncEntry *entry)
>
> {
> ...
> + foreach(lc, publications)
> + {
> + Publication *pub = lfirst(lc);
> +
> + /* No need to check column list publications */
> + if (is_column_list_publication(pub, entry->publish_as_relid))
>
> Are we ignoring column_list publications because for such publications
> the value of column_list prevails and we ignore
> 'publish_generated_columns' value? If so, it is not clear from the
> comments.
Yes column takes precedence over publish_generated_columns value, so
column list publications are skipped. Modified the comments
accordingly.
> 5.
> /* Initialize the column list */
> pgoutput_column_list_init(data, rel_publications, entry);
> +
> + /* Initialize publish generated columns value */
> + pgoutput_pubgencol_init(data, rel_publications, entry);
> +
> + /*
> + * Check if there is conflict with the columns selected for the
> + * publication.
> + */
> + check_conflicting_columns(rel_publications, entry);
> }
>
> It looks odd to check conflicting column lists among publications
> twice once in pgoutput_column_list_init() and then in
> check_conflicting_columns(). Can we merge those?
Modified it to check from pgoutput_column_list_init
The v42 version patch attached at [1] has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CALDaNm2wFZRzSJLcNi_uMZcSUWuZ8%2Bkktc0n3Nfw9Fdti9WbVA%40mail.gmail.com
Regards,
Vignesh
