From: | Shubham Khanna <khannashubham1197(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, Rajendra Kumar Dangwal <dangwalrajendra888(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, euler(at)eulerto(dot)com |
Subject: | Re: Pgoutput not capturing the generated columns |
Date: | 2024-10-25 14:10:56 |
Message-ID: | CAHv8Rj+ZyoWAHQO1XuP1sex7rMALcOs-wM6Oh5WHW+DJBE-Pkw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Oct 25, 2024 at 3:54 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Oct 25, 2024 at 12:07 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Oct 24, 2024 at 8:50 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > The v42 version patch attached at [1] has the changes for the same.
> > >
> >
> > Some more comments:
> >
>
> 1.
> +pgoutput_pubgencol_init(PGOutputData *data, List *publications,
> + RelationSyncEntry *entry)
>
> Can we name it as check_and_init_gencol? I don't know if it is a good
> idea to append a prefix pgoutput for local functions. It is primarily
> used for exposed functions from pgoutput.c. I see that in a few cases
> we do that for local functions as well but that is not a norm.
>
> A related point:
> + /* Initialize publish generated columns value */
> + pgoutput_pubgencol_init(data, rel_publications, entry);
>
> Accordingly change this comment to something like: "Check whether to
> publish to generated columns.".
>
Fixed.
> 2.
> +/*
> + * Returns true if the relation has column list associated with the
> + * publication, false if the relation has no column list associated with the
> + * publication.
> + */
> +bool
> +is_column_list_publication(Publication *pub, Oid relid)
> ...
> ...
>
> How about naming the above function as has_column_list_defined()?
> Also, you can write the above comment as: "Returns true if the
> relation has column list associated with the publication, false
> otherwise."
>
Fixed.
> 3.
> + /*
> + * The column list takes precedence over pubgencols, so skip checking
> + * column list publications.
> + */
> + if (is_column_list_publication(pub, entry->publish_as_relid))
>
> Let's change this comment to: "The column list takes precedence over
> publish_generated_columns option. Those will be checked later, see
> pgoutput_column_list_init."
>
Fixed.
The v43 version patch attached at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8RjJJJRzy83tG0nB90ivYcp7sFKTU%3D_BcQ-nUZ7VbHFwceA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
From | Date | Subject | |
---|---|---|---|
Next Message | jian he | 2024-10-25 14:26:48 | Re: altering a column's collation leaves an invalid foreign key |
Previous Message | Shubham Khanna | 2024-10-25 14:04:07 | Re: Pgoutput not capturing the generated columns |