Re: proposal: schema variables

st 23. 10. 2024 v 6:11 odesílatel Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
napsal:

> I have gone over patch 3 from the set and worked on the comments.
>
> Apart from that, I have modified your patch as follows:
>
> > +/*
> > + * pg_session_variables - designed for testing
> > + *
> > + * This is a function designed for testing and debugging. It returns
> the
> > + * content of sessionvars as-is, and can therefore display entries about
> > + * session variables that were dropped but for which this backend didn't
> > + * process the shared invalidations yet.
> > + */
> > +Datum
> > +pg_session_variables(PG_FUNCTION_ARGS)
> > +{
> > +#define NUM_PG_SESSION_VARIABLES_ATTS 8
> > +
> > + 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

>
> > + while ((svar = (SVariable) hash_seq_search(&status)) !=
> NULL)
> > + {
> > + Datum
> values[NUM_PG_SESSION_VARIABLES_ATTS];
> > + bool
> nulls[NUM_PG_SESSION_VARIABLES_ATTS];
> > + HeapTuple tp;
> > + bool var_is_valid = false;
> > +
> > + 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

> > + 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.

>
> There is a dependency between the variable and the type, but if a
> concurrent
> session drops both the variable and the data type, the non-existing type ID
> would still show up in the function output.
> Even worse, the OID could have been reused for a different type since.
>

I agree so usage `select typid::regtype, ... from pg_session_variables(.. `

can be dangerous, but it is the reason why we have there typname column.

Showing 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'll send the attachment with merging changes for patch 4

Regards

Pavel

>
> I am attaching just patch number 3 and leave you to adapt the patch set,
> but I don't think any of the other patches should be affected.
>

The comments from your patch are merged

>
> Yours,
> Laurenz Albe
>