Обсуждение: Added missing invalidations for all tables publication

Hi,

Relation invalidation was missing in case of create publication and
drop publication of "FOR ALL TABLES" publication, added so that the
publication information can be rebuilt. Without these invalidation
update/delete operations on the relation will be successful in the
publisher which will later result in conflict in the subscriber.
Thanks to Amit for identifying the issue at [1]. Attached patch has
the fix for the same.
Thoughts?

[1] - https://www.postgresql.org/message-id/CAA4eK1LtTXMqu-UbcByjHw%2BaKP38t4%2Br7kyKnmBQMA-__9U52A%40mail.gmail.com

Regards,
Vignesh

From Tuesday, August 31, 2021 1:10 AM vignesh C <vignesh21@gmail.com> wrote:
> Hi,
> 
> Relation invalidation was missing in case of create publication and drop
> publication of "FOR ALL TABLES" publication, added so that the publication
> information can be rebuilt. Without these invalidation update/delete
> operations on the relation will be successful in the publisher which will later
> result in conflict in the subscriber.
> Thanks to Amit for identifying the issue at [1]. Attached patch has the fix for the
> same.
> Thoughts?

I have one comment about the testcase in the patch.

+-- Test cache invalidation FOR ALL TABLES publication
+SET client_min_messages = 'ERROR';
+CREATE TABLE testpub_tbl4(a int);
+CREATE PUBLICATION testpub_foralltables FOR ALL TABLES;
+RESET client_min_messages;
+-- fail missing REPLICA IDENTITY
+UPDATE testpub_tbl4 set a = 2;
+ERROR:  cannot update table "testpub_tbl4" because it does not have a replica identity and publishes updates
+HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
+DROP PUBLICATION testpub_foralltables;

The above testcases can pass without the code change in the patch, is it better
to add a testcase which can show different results after applying the patch ?

Best regards,
Hou zj

On Tue, Aug 31, 2021 at 7:40 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> From Tuesday, August 31, 2021 1:10 AM vignesh C <vignesh21@gmail.com> wrote:
> > Hi,
> >
> > Relation invalidation was missing in case of create publication and drop
> > publication of "FOR ALL TABLES" publication, added so that the publication
> > information can be rebuilt. Without these invalidation update/delete
> > operations on the relation will be successful in the publisher which will later
> > result in conflict in the subscriber.
> > Thanks to Amit for identifying the issue at [1]. Attached patch has the fix for the
> > same.
> > Thoughts?
>
> I have one comment about the testcase in the patch.
>
> +-- Test cache invalidation FOR ALL TABLES publication
> +SET client_min_messages = 'ERROR';
> +CREATE TABLE testpub_tbl4(a int);
> +CREATE PUBLICATION testpub_foralltables FOR ALL TABLES;
> +RESET client_min_messages;
> +-- fail missing REPLICA IDENTITY
> +UPDATE testpub_tbl4 set a = 2;
> +ERROR:  cannot update table "testpub_tbl4" because it does not have a replica identity and publishes updates
> +HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
> +DROP PUBLICATION testpub_foralltables;
>
> The above testcases can pass without the code change in the patch, is it better
> to add a testcase which can show different results after applying the patch ?

Thanks for the comment, I have slightly modified the test case which
will fail without the patch. Attached v2 patch which has the changes
for the same.

Regards,
Vignesh

At Tue, 31 Aug 2021 08:31:05 +0530, vignesh C <vignesh21@gmail.com> wrote in 
> On Tue, Aug 31, 2021 at 7:40 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> Thanks for the comment, I have slightly modified the test case which
> will fail without the patch. Attached v2 patch which has the changes
> for the same.

The test works fine. The code looks fine for me except one minor
cosmetic flaw.

+    if (!HeapTupleIsValid(tup))
+        elog(ERROR, "cache lookup failed for publication %u",
+             pubid);

The last two lines don't need to be separated. ((Almost) All other
instance of the same error is written that way.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Aug 31, 2021 at 2:00 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Tue, 31 Aug 2021 08:31:05 +0530, vignesh C <vignesh21@gmail.com> wrote in
> > On Tue, Aug 31, 2021 at 7:40 AM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > Thanks for the comment, I have slightly modified the test case which
> > will fail without the patch. Attached v2 patch which has the changes
> > for the same.
>
> The test works fine. The code looks fine for me except one minor
> cosmetic flaw.
>
> +       if (!HeapTupleIsValid(tup))
> +               elog(ERROR, "cache lookup failed for publication %u",
> +                        pubid);
>
> The last two lines don't need to be separated. ((Almost) All other
> instance of the same error is written that way.
>

Thanks for the comments, the attached v3 patch has the changes for the same.

Regards,
Vignesh

On Tue, Aug 31, 2021 at 8:54 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Aug 31, 2021 at 2:00 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
>
> Thanks for the comments, the attached v3 patch has the changes for the same.
>

I think this bug should be fixed in back branches (till v10). OTOH, as
this is not reported by any user and we have found it during code
review so it seems either users don't have an exact use case or they
haven't noticed this yet. What do you people think about
back-patching?

Attached, please find a slightly updated patch with minor changes. I
have slightly changed the test to make it more meaningful. If we
decide to back-patch this, can you please test this on back-branches?

-- 
With Regards,
Amit Kapila.

From Mon, Sep 6, 2021 1:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Aug 31, 2021 at 8:54 PM vignesh C <vignesh21@gmail.com> wrote:
> > Thanks for the comments, the attached v3 patch has the changes for the
> > same.
> >
> 
> I think this bug should be fixed in back branches (till v10). OTOH, as this is not
> reported by any user and we have found it during code review so it seems
> either users don't have an exact use case or they haven't noticed this yet. What
> do you people think about back-patching?

Personally, I think it's ok to back-patch.

> Attached, please find a slightly updated patch with minor changes. I have
> slightly changed the test to make it more meaningful.

The patch looks good to me.

Best regards,
Hou zj

> From Mon, Sep 6, 2021 1:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Tue, Aug 31, 2021 at 8:54 PM vignesh C <vignesh21@gmail.com> wrote:
> > > Thanks for the comments, the attached v3 patch has the changes for
> > > the same.
> > >
> >
> > I think this bug should be fixed in back branches (till v10). OTOH, as
> > this is not reported by any user and we have found it during code
> > review so it seems either users don't have an exact use case or they
> > haven't noticed this yet. What do you people think about back-patching?
> 
> Personally, I think it's ok to back-patch.

I found that the patch cannot be applied to back-branches(v10-v14) cleanly,
so, I generate the patches for back-branches. Attached, all the patches have
passed regression test.

Best regards,
Hou zj

On Wed, Sep 8, 2021 at 7:57 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> > From Mon, Sep 6, 2021 1:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > On Tue, Aug 31, 2021 at 8:54 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > Thanks for the comments, the attached v3 patch has the changes for
> > > > the same.
> > > >
> > >
> > > I think this bug should be fixed in back branches (till v10). OTOH, as
> > > this is not reported by any user and we have found it during code
> > > review so it seems either users don't have an exact use case or they
> > > haven't noticed this yet. What do you people think about back-patching?
> >
> > Personally, I think it's ok to back-patch.
>
> I found that the patch cannot be applied to back-branches(v10-v14) cleanly,
> so, I generate the patches for back-branches. Attached, all the patches have
> passed regression test.
>

Pushed!

-- 
With Regards,
Amit Kapila.

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Wed, Sep 8, 2021 at 7:57 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
>> I found that the patch cannot be applied to back-branches(v10-v14) cleanly,
>> so, I generate the patches for back-branches. Attached, all the patches have
>> passed regression test.

> Pushed!

Shouldn't the CF entry for this be closed? [1]

            regards, tom lane

[1] https://commitfest.postgresql.org/34/3311/

On Sat, Sep 11, 2021 at 11:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Wed, Sep 8, 2021 at 7:57 AM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> >> I found that the patch cannot be applied to back-branches(v10-v14) cleanly,
> >> so, I generate the patches for back-branches. Attached, all the patches have
> >> passed regression test.
>
> > Pushed!
>
> Shouldn't the CF entry for this be closed? [1]
>

Yes, and I have done that now.

-- 
With Regards,
Amit Kapila.