On Tue, Jan 18, 2022 at 10:23 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Tue, Jan 18, 2022 at 2:31 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Tue, Jan 18, 2022 at 8:41 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > > > > > > On Tue, Jan 18, 2022 at 2:31 AM houzj.f...@fujitsu.com > > > <houzj.f...@fujitsu.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.
