Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1t4CmF-00Gy9K-KQ for pgsql-hackers@arkaria.postgresql.org; Fri, 25 Oct 2024 05:22:23 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1t4CmD-00CMNp-Ps for pgsql-hackers@arkaria.postgresql.org; Fri, 25 Oct 2024 05:22:22 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1t4CmD-00CMNg-BX for pgsql-hackers@lists.postgresql.org; Fri, 25 Oct 2024 05:22:21 +0000 Received: from mail-yb1-xb32.google.com ([2607:f8b0:4864:20::b32]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1t4Cm6-002joe-Th for pgsql-hackers@lists.postgresql.org; Fri, 25 Oct 2024 05:22:20 +0000 Received: by mail-yb1-xb32.google.com with SMTP id 3f1490d57ef6-e2e2ef2e906so1950649276.2 for ; Thu, 24 Oct 2024 22:22:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1729833734; x=1730438534; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=9CutTDMfCzcWUKrXut5qCWWKwUIaBECOD4fahkw18lA=; b=cNPBIdV/tG+AU+CaCB3WNzV8m0Z1iQX60Aas7l+CMtBT9Nn8yaUleYQMhp7gECjkGE NcKnr8dBza+po5v9wRegKdtT2nEAerdM/X/Y7/HGE/iFNXTN+TNpXAxBzvSvbmqmQb9w LPJj1MY2usZZvkyciYDZ8w4pnbr47EmYFLlp0uN6B5Hl1aDUrE+bYdZ6zJoE+w06UhLG mG7WlpgzDX9d+7pdslvtlZlT8oV17ERdu/NpUQi1BHimo2C5OjcQWxd8ZkYnZaOpHNpn /2tf/vJI7Jj91X/sWZhmp4g6rp9fCyoVAKH2Zo9+n5MdZY9ZJwtWBRoTvr4ciz57JP6D +6CA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729833734; x=1730438534; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=9CutTDMfCzcWUKrXut5qCWWKwUIaBECOD4fahkw18lA=; b=NqnCk/wDnMakjLJEpCcsdk61jkwc2LRxq/q6W4OjK82GYsCKyCvrlspCvsvYe4yJj5 ClYJek1Vx1AEtEgD5c2sb3cWgDg581V7QsHeBbaoufAni38Gs+q6IFqJfnd+8VCw8PQ/ e29tBfGVvzqiMrir0zJwqGxktfMfMSs/CDnS7E7iHmRXAPmPsQJmkmyax/SZHmsH+d0j u87Q303ZzXaVi/NQ6bkwP8bBYvB9UQ7mW4EwQnbS6hh3HsZ3CU4dU5I6NLba+hfEanpl ceX43US+6tBcUsly3kLU7428jtka9raxgB2lehto09yC9LQcLDAjLlV6KjRk6azCsUoy Ilfg== X-Forwarded-Encrypted: i=1; AJvYcCWHGe/f9EsS+0B6N1G5FFk5gLm1nArJ2m3mjrokpzl1qCuAyklT4bxKHA44ndsdq+Ia5ANorvxhwLie0pMe@lists.postgresql.org X-Gm-Message-State: AOJu0YwJSCWGcB5ywooh13d9UGJ/6oZLieyadSO2fmml46zfzn/23pUU 2+gPNdxJwxSCbSGA9sdHv5FeSEbUkD+DcC8zitJz9G3Bb7Ht4jP9usipgYPpidqHroGrSQtzpMp tsyuZ6cXTYRgFAJLhmO7yqRDq/8Y= X-Google-Smtp-Source: AGHT+IHrrLnW4pVv+A82dRlFq26RaQTGvV5EtUk8febULlLM5hJI1xZOOKaxhEJ0jbGklrP9ICKV1AECpWiAi81naZA= X-Received: by 2002:a05:6902:2b8a:b0:e2b:d131:f293 with SMTP id 3f1490d57ef6-e2e3a6d3e2amr8942399276.51.1729833733854; Thu, 24 Oct 2024 22:22:13 -0700 (PDT) MIME-Version: 1.0 References: <20200924035637.GF28585@paquier.xyz> <20201001033824.GC8130@paquier.xyz> <51a9a68e8a998d04df17417d45c1dbd4@xs4all.nl> <89817942c99da01cd5e7850fe418436b@xs4all.nl> <56ca532c37eb0b540961f74a7bd5db39@xs4all.nl> <8181bd3abc647bdae5a4f78e71e62478a98c75f4.camel@cybertec.at> <9e67d49deb18270eddb95e602c83f02b98459843.camel@cybertec.at> <3b662dc5b615d4c20a55e8e2fbe6fc00fe00609d.camel@cybertec.at> <6996931e8c9edf3b82223e74e92326a7ed06c1d6.camel@cybertec.at> <67aa68a7e6dfb44c0cbbdf7f97cadfede4269ce5.camel@cybertec.at> <04ec666686e9e21cb515617df06885c66f3d34ce.camel@cybertec.at> <3850a85012d040827b10193189edbe2c23a64f8f.camel@cybertec.at> In-Reply-To: From: Pavel Stehule Date: Fri, 25 Oct 2024 07:21:34 +0200 Message-ID: Subject: Re: proposal: schema variables To: Laurenz Albe Cc: Erik Rijkers , Michael Paquier , Amit Kapila , DUVAL REMI , PostgreSQL Hackers Content-Type: multipart/alternative; boundary="0000000000003a1e950625464dc4" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000003a1e950625464dc4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable st 23. 10. 2024 v 6:11 odes=C3=ADlatel Laurenz Albe 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 abo= ut > > + * 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 =3D (SVariable) hash_seq_search(&status)) != =3D > NULL) > > + { > > + Datum > values[NUM_PG_SESSION_VARIABLES_ATTS]; > > + bool > nulls[NUM_PG_SESSION_VARIABLES_ATTS]; > > + HeapTuple tp; > > + bool var_is_valid =3D 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] =3D {}; > > 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 `=3D {} ` in code > > + values[0] =3D ObjectIdGetDatum(svar->varid); > > + values[3] =3D 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 > --0000000000003a1e950625464dc4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


st 23. 10. 2024 v=C2= =A06:11 odes=C3=ADlatel 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.=C2=A0 It re= turns the
> + * content of sessionvars as-is, and can therefore display entries ab= out
> + * session variables that were dropped but for which this backend did= n't
> + * process the shared invalidations yet.
> + */
> +Datum
> +pg_session_variables(PG_FUNCTION_ARGS)
> +{
> +#define NUM_PG_SESSION_VARIABLES_ATTS 8
> +
> +=C2=A0 =C2=A0 =C2=A0elog(DEBUG1, "pg_session_variables start&quo= t;);

I don't think that message is necessary, particularly with DEBUG1.
I have removed this message and the "end" message as well.

removed
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0while ((svar =3D (SVa= riable) hash_seq_search(&status)) !=3D NULL)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0Datum=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0values[NUM_PG_SESSION_= VARIABLES_ATTS];
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0bool=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nulls[NUM_PG_SESSION_V= ARIABLES_ATTS];
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0HeapTuple=C2=A0 =C2=A0 =C2=A0 =C2=A0tp;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0bool=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 var_is_valid =3D false= ;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0memset(values, 0, sizeof(values));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0memset(nulls, 0, sizeof(nulls));

Instead of explicitly zeroing out the arrays, I have used an empty initiali= zer
in the definition, like

=C2=A0 bool=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nulls[NUM_PG_SESSION_VARIABLE= S_ATTS] =3D {};

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 `=3D {} ` in code


> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0values[0] =3D ObjectIdGetDatum(svar->varid);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0values[3] =3D 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 ide= a was using typid as hint identification of deleted variables. The possibil= ity that this id will not be consistent for the current catalogue was expec= ted. And it
is a reason why the result type is just Oid and not r= egtype. Without it, pg_session_variables shows just empty rows (except oid)= for dropped not yet purged variables.
=C2=A0

There is a dependency between the variable and the type, but if a concurren= t
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.
<= /blockquote>

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 onl= y when variable is valid, and using regtype as output type, remove typname<= /div>

What do you prefer?

= I'll send the attachment with merging changes for patch 4

Regards

Pavel
=C2=A0=

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

=C2=A0

Yours,
Laurenz Albe
--0000000000003a1e950625464dc4--