Re: Disallow UPDATE/DELETE on table with unpublished generated column as REPLICA IDENTITY

On Tue, 19 Nov 2024 at 10:22, vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 19 Nov 2024 at 00:36, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Mon, 18 Nov 2024 at 19:19, vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Mon, 18 Nov 2024 at 13:07, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > > >
> > > > Thanks for providing the comments.
> > > >
> > > > On Sat, 16 Nov 2024 at 17:29, vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > I have attached the updated version of the patch.
> > >
> > > Few comments:
> > > 1) We have the following check for cols validation and rf validation:
> > >     /*
> > >      * If we know everything is replicated and the column list is invalid
> > >      * for update and delete, there is no point to check for other
> > >      * publications.
> > >      */
> > >     if (pubdesc->pubactions.pubinsert && pubdesc->pubactions.pubupdate &&
> > >       pubdesc->pubactions.pubdelete && pubdesc->pubactions.pubtruncate &&
> > >       !pubdesc->cols_valid_for_update && !pubdesc->cols_valid_for_delete)
> > >       break;
> > >
> > > Should we do this for replident_valid_for_update and
> > > replident_valid_for_delete also?
> > >
> > Yes, we can add this check.
> >
> > > 2) This variable is not required, there is a warning:
> > > publicationcmds.c: In function ‘replident_has_unpublished_gen_col’:
> > > publicationcmds.c:486:41: warning: unused variable ‘x’ [-Wunused-variable]
> > Fixed
> >
> > I have fixed the comments and attached an updated patch.
>
> To ensure easy backtracking after the patch is committed, we should
> include a brief explanation for the test removal in the commit
> message:
> diff --git a/src/test/subscription/t/100_bugs.pl
> b/src/test/subscription/t/100_bugs.pl
> index cb36ca7b16..794b928f50 100644
> --- a/src/test/subscription/t/100_bugs.pl
> +++ b/src/test/subscription/t/100_bugs.pl
> @@ -377,8 +377,8 @@ $node_publisher->safe_psql('postgres', "DROP
> PUBLICATION tap_pub_sch");
>  $node_publisher->stop('fast');
>  $node_subscriber->stop('fast');
>
> -# The bug was that when the REPLICA IDENTITY FULL is used with dropped or
> -# generated columns, we fail to apply updates and deletes
> +# The bug was that when the REPLICA IDENTITY FULL is used with dropped
> +# we fail to apply updates and deletes
>  $node_publisher->rotate_logfile();
>  $node_publisher->start();
>
> @@ -389,18 +389,14 @@ $node_publisher->safe_psql(
>   'postgres', qq(
>   CREATE TABLE dropped_cols (a int, b_drop int, c int);
>   ALTER TABLE dropped_cols REPLICA IDENTITY FULL;
> - CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5
> * a) STORED, c int);
> - ALTER TABLE generated_cols REPLICA IDENTITY FULL;
> - CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols, generated_cols;
> + CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols;
>

I noticed that we can add 'publish_generated_columns = true' for the
case of generated column. So we won't need to remove the test. I have
made the changes in v9 patch [1].

[1]: https://www.postgresql.org/message-id/CANhcyEVCqrSYxAg_s99VYevUc4F-Lb9XowWUC2E5RG0i8RtZwA%40mail.gmail.com

Thanks and Regards,
Shlok Kyal