On Fri, 2024-10-25 at 07:21 +0200, Pavel Stehule wrote:
> > > + elog(DEBUG1, "pg_session_variables start");
> >
> > I don't think that message is necessary, particularly with DEBUG1.
> > I have removed this message and the "end" message as well.
>
> removed
Thanks.
> > > + memset(values, 0, sizeof(values));
> > > + memset(nulls, 0, sizeof(nulls));
> >
> > Instead of explicitly zeroing out the arrays, I have used an empty initializer
> > in the definition, like
> >
> > bool nulls[NUM_PG_SESSION_VARIABLES_ATTS] = {};
> >
> > That should have the same effect.
> > If you don't like that, I have no real problem with your original code.
>
> I prefer the original way - minimally it is a common pattern. I didn't find any usage of `= {} ` in code
That's alright by me.
> > > + values[0] = ObjectIdGetDatum(svar->varid);
> > > + values[3] = ObjectIdGetDatum(svar->typid);
> >
> > You are using the type ID without checking if it exists in the catalog.
> > I think that is a bug.
>
> The original idea was using typid as hint identification of deleted variables. The possibility
> that this id will not be consistent for the current catalogue was expected. And it
> is a reason why the result type is just Oid and not regtype. Without it, pg_session_variables
> shows just empty rows (except oid) for dropped not yet purged variables.
I see your point. It is for testing and debugging only.
>
> owing typid has some information value, but I don't think it is absolutely necessary. I see some possible changes:
>
> 1. no change
> 2. remove typid column
> 3. show typid only when variable is valid, and using regtype as output type, remove typname
>
> What do you prefer?
I'd say leave it as it is. I agree that it is not dangerous, and if it is intentional that
non-existing type IDs might be displayed, I have no problem with it.
Yours,
Laurenz Albe
