Re: Add an option to skip loading missing publication to avoid logical replication failure

On Tue, 4 Mar 2025 at 12:22, vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 3 Mar 2025 at 16:41, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Mar 3, 2025 at 2:30 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Tue, 25 Feb 2025 at 15:32, vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > The attached script has the script that was used for testing. Here the
> > > > NUM_RECORDS count should be changed accordingly for each of the tests
> > > > and while running the test with the patch change uncomment the drop
> > > > publication command.
> > >
> > > I have done further analysis on the test and changed the test to
> > > compare it better with HEAD. The execution time is in milliseconds.
> > > Brach/records  |  100     |  1000   |  10000    |  100000  |  1000000
> > > Head               |   10.43  |  15.86  |   64.44    |  550.56  |   8991.04
> > > Patch              |   11.35  |  17.26   |   73.50    |  640.21  |  10104.72
> > > % diff              |   -8.82  |  -8.85    |   -14.08   |   -16.28  |  -12.38
> > >
> > > There is a  performance degradation in the range of 8.8 to 16.2 percent.
> > >
> >
> > - /* Validate the entry */
> > - if (!entry->replicate_valid)
> > + /*
> > + * If the publication is invalid, check for updates.
> > + * This optimization ensures that the next block, which queries the system
> > + * tables and builds the relation entry, runs only if a new publication was
> > + * created.
> > + */
> > + if (!publications_valid && data->publications)
> > + {
> > + bool skipped_pub = false;
> > + List    *publications;
> > +
> > + publications = LoadPublications(data->publication_names, &skipped_pub);
> >
> > The publications_valid flag indicates whether the publications cache
> > is valid or not; the flag is set to false for any invalidation in the
> > pg_publication catalog. I wonder that instead of using the same flag
> > what if we use a separate publications_skipped flag? If that works,
> > you don't even need to change the current location where we
> > LoadPublications.
>
> There is almost negligible dip with the above suggested way, the test
> results for the same is given below(execution time is in milli
> seconds):
> Brach/records  |  100     |  1000   |  10000    |  100000 |  1000000
> Head               |   10.25  |  15.85  |   65.53    |  569.15  |  9194.19
> Patch              |   10.25  |  15.84  |   65.91    |  571.75  |  9208.66
> % diff              |      0.00 |    0.06  |    -0.58    |     -0.46  |    -0.16
>
> There is a performance dip in the range of 0 to 0.58 percent.
> The attached patch has the changes for the same. The test script used
> is also attached.

On further thinking, I felt the use of publications_updated variable
is not required we can use publications_valid itself which will be set
if the publication system table is invalidated. Here is a patch for
the same.

Regards,
Vignesh