Re: proposal: schema variables - Mailing list pgsql-hackers
| From | Pavel Stehule |
|---|---|
| Subject | Re: proposal: schema variables |
| Date | |
| Msg-id | CAFj8pRAxSiA68tS=ky64A9fhKGstRVpgQfPotNmMF0-4GuisRQ@mail.gmail.com Whole thread Raw |
| In response to | Re: proposal: schema variables (Pavel Stehule <pavel.stehule@gmail.com>) |
| Responses |
Re: proposal: schema variables
|
| List | pgsql-hackers |
st 23. 10. 2024 v 6:11 odesílatel Laurenz Albe <laurenz.albe@cybertec.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
pgsql-hackers by date: