On Mon, Jul 29, 2024 at 4:14 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Mon, 29 Jul 2024 at 17:19, Peter Smith <smithpb2250@gmail.com> wrote:
> > Please see attached fix v2
>
> It's likely also worth fixing the incorrect header comment for
> publication_translate_columns() while fixing this. "and a Bitmapset
> with them;" seems to be incorrect and not all that well phrased.
>
Yeah, I noticed that appeared misleading.
> On the whole, I think the API of these functions could be improved as
> it's made the fix for this bug more convoluted than it needs to be.
I agree. It is better than it was, but is still jumping through some
hoops a bit to get the bitmap.
> It would be much nicer if publication_translate_columns() returned a
> Bitmapset *. That would reduce the code size by a dozen or so lines
> as you could get rid of the qsort() and the compare_int16 function.
> Converting a bitmapset to a vector will lead to a naturally sorted
> vector. Doing this would mean having to invent a function that takes a
> Bitmapset to do the job of buildint2vector, which is effectively, the
> inverse of pub_collist_to_bitmapset(). You could probably also strip
> about 7 lines out of pub_collist_to_bitmapset() by just doing
> Bitmapset *result = columns; It probably won't change the compiled
> code, however.
>
> I'm on the fence if this should be done as part of this bug fix. The
> reason I think it might is that you're changing
> publication_translate_columns() to be non-static, and if the above API
> change gets done later, that's then changing the API of an existing
> external function. The reason against is that it's more risky to do in
> the back branches as it's changing more code. What do others think?
>
I'll wait until tomorrow in case there are other opinions as to
whether that belongs in this patch or elsewhere..
TBH, I was unsure if this error bugfix patch qualified to have
backpatches or not. Although it is a "bug" it was not impacting
anybody because it is only substituting one error msg for another
nicer error msg.
> Is it worth adding an ALTER PUBLICATION test with a duplicate column too?
+1 to do that.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
