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.
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.
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?
Is it worth adding an ALTER PUBLICATION test with a duplicate column too?
David
