Hello Vaibhav,
I wonder why is Subscription->publications a list of String rather than
a list of C strings. That's something you'd see in a Node structure,
but Subscription is not a node, so this seems wasteful and pointless.
This is of course not the fault of your patch, but the fact that your
patch feels the need to expose the textarray_to_stringlist() auxiliary
function made me wonder about it. I think that's not a great function
to expose, at least not from pg_subscription.h, so maybe we should
instead think about getting rid of the String nodes from there and see
about making this whole thing simpler. On the other hand, we already
have function strlist_to_textarray() declared in objectaddress.h, which
is kinda the inverse of this ... Looking further, I wonder why we have
publicationListToArray() when it seems strlist_to_textarray() is likely
to fit the bill, with a couple of tweaks --- assuming we want to keep
using String nodes in Subscription, which I doubt.
Oh, we also have textarray_to_strvaluelist() which is essentially
identical, but also static. If we're making one of them non-static,
then for sure let's remove the other one. But maybe what we really need
is a third one to use in ruleutils, and expose neither? (I think if we
get rid of the String around Subscription->publications, that's likely
what I'd do, since they'd be mostly trivial wrappers around
deconstruct_array_builtin.)
Anyway, I guess this is a long-winded way of saying that I don't think
making textarray_to_stringlist() non-static is a great idea. At least
not where you're doing it. I would start this with a 0001 patch that
gets rid of String usage there, and then the rest of your function on
top of that.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"
