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:

Previous
From: Peter Smith
Date:
Subject: Re: Refactor to use common function 'get_publications_str'.
Next
From: Thomas Munro
Date:
Subject: Re: Docs Build in CI failing with "failed to load external entity"