On Mon, Feb 26, 2024 at 09:38:06AM +0100, Matthias van de Meent wrote:
> On Mon, 26 Feb 2024 at 09:11, Michael Paquier <michael@paquier.xyz> wrote:
>> Thanks, I've applied these two for now. I'll reply to the rest
>> tomorrow or so.
>
> Huh, that's surprising to me. I'd expected this to get at least a
> final set of patches before they'd get committed.
FWIW, these refactoring pieces just make sense taken independently,
IMHO. I don't think that the rest of the patch set is going to make
it into v17, because there's no agreement about the layer we want,
which depend on the use cases we want to solve. Perhaps 0001 or 0004
could be salvaged. 0005~ had no real design discussion, so it's good
for 18~ as far as I am concerned. That's something that would be fit
for an unconference session at the next pgconf in Vancouver, in
combination with what we should do to support sequences across logical
replication setups.
> After a quick check
> 6e951bf seems fine, but I do have some nits on 449e798c:
Thanks.
>> +/* ----------------
>> + * validate_relation_kind - check the relation's kind
>> + *
>> + * Make sure relkind is from an index
>
> Shouldn't this be "... from a sequence"?
Right, will fix.
>> + * ----------------
>> + */
>> +static inline void
>> +validate_relation_kind(Relation r)
>
> Shouldn't this be a bit more descriptive than just
> "validate_relation_kind"? I notice this is no different from how this
> is handled in index.c and table.c, but I'm not a huge fan of shadowing
> names, even with static inlines functions.
Not sure that it matters much, TBH. This is local to sequence.c.
>> -ERROR: "serialtest1" is not a sequence
>> +ERROR: cannot open relation "serialtest1"
>> +DETAIL: This operation is not supported for tables.
>
> We seem to lose some details here: We can most definitely open tables.
> We just can't open them while treating them as sequences, which is not
> mentioned in the error message.
I am not sure to agree with that. The user already knows that he
should be dealing with a sequence based on the DDL used, and we gain
information about the relkind getting manipulated here.
--
Michael