Re: row filtering for logical replication

On Tue, Jan 18, 2022 at 10:23 AM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Tue, Jan 18, 2022 at 2:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Jan 18, 2022 at 8:41 AM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Jan 18, 2022 at 2:31 AM houzj(dot)fnst(at)fujitsu(dot)com
> > > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > >
> > > > > (2) GetTopMostAncestorInPublication
> > > > > Is there a reason why there is no "break" after finding a
> > > > > topmost_relid? Why keep searching and potentially overwrite a
> > > > > previously-found topmost_relid? If it's intentional, I think that a
> > > > > comment should be added to explain it.
> > > >
> > > > The code was moved from get_rel_sync_entry, and was trying to get the
> > > > last oid in the ancestor list which is published by the publication. Do you
> > > > have some suggestions for the comment ?
> > > >
> > >
> > > Maybe the existing comment should be updated to just spell it out like that:
> > >
> > > /*
> > > * Find the "topmost" ancestor that is in this publication, by getting the
> > > * last Oid in the ancestors list which is published by the publication.
> > > */
> > >
> >
> > I am not sure that is helpful w.r.t what Peter is looking for as that
> > is saying what code is doing and he wants to know why it is so? I
> > think one can understand this by looking at get_partition_ancestors
> > which will return the top-most ancestor as the last element. I feel
> > either we can say see get_partition_ancestors or maybe explain how the
> > ancestors are stored in this list.
> >
>
> (note: I asked the original question about why there is no "break", not Peter)
>

Okay.

> Maybe instead, an additional comment could be added to the
> GetTopMostAncestorInPublication function to say "Note that the
> ancestors list is ordered such that the topmost ancestor is at the end
> of the list".
>

I am fine with this and I see that Hou-San already used this in the
latest version of patch.

> Unfortunately the get_partition_ancestors function
> currently doesn't explicitly say that the topmost ancestors are
> returned at the end of the list (I guess you could conclude it by then
> looking at get_partition_ancestors_worker code which it calls).
> Also, this leads me to wonder if searching the ancestors list
> backwards might be better here, and break at the first match?
>

I am not sure of the gains by doing that and anyway, that is a
separate topic of discussion as it is an existing code.

--
With Regards,
Amit Kapila.