Обсуждение: pgsql: Cache by-reference missing values in a long lived context
Cache by-reference missing values in a long lived context Attribute missing values might be needed past the lifetime of the tuple descriptors from which they are extracted. To avoid possibly using pointers for by-reference values which might thus be left dangling, we cache a datumCopy'd version of the datum in the TopMemoryContext. Since we first search for the value this only needs to be done once per session for any such value. Original complaint from Tom Lane, idea for mitigation by Andrew Dunstan, tweaked by Tom Lane. Backpatch to version 11 where missing values were introduced. Discussion: https://postgr.es/m/1306569.1687978174@sss.pgh.pa.us Branch ------ REL_11_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/2d13dab048a7e0777875529620f81d32ce57dfd8 Modified Files -------------- src/backend/access/common/heaptuple.c | 91 ++++++++++++++++++++++++++++++++++- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 91 insertions(+), 1 deletion(-)
Andrew Dunstan <andrew@dunslane.net> writes:
> Cache by-reference missing values in a long lived context
The v11 version of this patch is causing a compiler warning for me:
In file included from heaptuple.c:58:
heaptuple.c: In function 'missing_hash':
heaptuple.c:97:3: warning: implicit declaration of function 'hash_any'; did you mean 'hash_stats'?
[-Wimplicit-function-declaration]
hash_any((const unsigned char *) entry->value, entry->len));
^~~~~~~~
../../../../src/include/postgres.h:471:38: note: in definition of macro 'DatumGetUInt32'
#define DatumGetUInt32(X) ((uint32) (X))
^
It seems to work anyway, but please fix.
regards, tom lane
On 2023-08-24 Th 11:27, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:Cache by-reference missing values in a long lived contextThe v11 version of this patch is causing a compiler warning for me: In file included from heaptuple.c:58: heaptuple.c: In function 'missing_hash': heaptuple.c:97:3: warning: implicit declaration of function 'hash_any'; did you mean 'hash_stats'? [-Wimplicit-function-declaration] hash_any((const unsigned char *) entry->value, entry->len)); ^~~~~~~~ ../../../../src/include/postgres.h:471:38: note: in definition of macro 'DatumGetUInt32' #define DatumGetUInt32(X) ((uint32) (X)) ^ It seems to work anyway, but please fix.
Sorry about that, fixed.
While we're about it, let's also fix these warnings which are seen on my systems building releases 11 and 12:
/home/andrew/bf/root/REL_11_STABLE/pgsql.build/../pgsql/src/backend/commands/foreigncmds.c:481:22: warning: ‘funcargtypes’ may be used uninitialized [-Wmaybe-uninitialized]
/home/andrew/bf/root/REL_12_STABLE/pgsql.build/../pgsql/src/backend/commands/foreigncmds.c:487:22: warning: ‘funcargtypes’ may be used uninitialized [-Wmaybe-uninitialized]
Maybe funcargtypes here should be initialized to { 0 } ?
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-08-24 Th 11:27, Tom Lane wrote:
>> The v11 version of this patch is causing a compiler warning for me:
> Sorry about that, fixed.
Thanks!
> While we're about it, let's also fix these warnings which are seen on my
> systems building releases 11 and 12:
> /home/andrew/bf/root/REL_11_STABLE/pgsql.build/../pgsql/src/backend/commands/foreigncmds.c:481:22:
> warning: ‘funcargtypes’ may be used uninitialized [-Wmaybe-uninitialized]
> /home/andrew/bf/root/REL_12_STABLE/pgsql.build/../pgsql/src/backend/commands/foreigncmds.c:487:22:
> warning: ‘funcargtypes’ may be used uninitialized [-Wmaybe-uninitialized]
> Maybe funcargtypes here should be initialized to { 0 } ?
Hm. It looks like we got rid of those warnings in v13 via dcb7d3caf:
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Branch: master Release: REL_13_BR [dcb7d3caf] 2019-11-12 17:06:58 -0300
Have LookupFuncName accept NULL argtypes for 0 args
I'm a little tempted to propose that a better solution is to
back-patch that patch. Removing the warning alone doesn't make
a very strong case for that, but there are other arguments:
* Somebody might back-patch code relying on the newer convention;
* All else being equal, it's better to keep the code in different
branches looking similar.
I'm not sure if those arguments justify a back-patch instead of
the localized hack you suggest.
regards, tom lane
On 2023-08-24 Th 16:57, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:On 2023-08-24 Th 11:27, Tom Lane wrote:The v11 version of this patch is causing a compiler warning for me:Sorry about that, fixed.Thanks!While we're about it, let's also fix these warnings which are seen on my systems building releases 11 and 12:/home/andrew/bf/root/REL_11_STABLE/pgsql.build/../pgsql/src/backend/commands/foreigncmds.c:481:22: warning: ‘funcargtypes’ may be used uninitialized [-Wmaybe-uninitialized] /home/andrew/bf/root/REL_12_STABLE/pgsql.build/../pgsql/src/backend/commands/foreigncmds.c:487:22: warning: ‘funcargtypes’ may be used uninitialized [-Wmaybe-uninitialized]Maybe funcargtypes here should be initialized to { 0 } ?Hm. It looks like we got rid of those warnings in v13 via dcb7d3caf: Author: Alvaro Herrera <alvherre@alvh.no-ip.org> Branch: master Release: REL_13_BR [dcb7d3caf] 2019-11-12 17:06:58 -0300 Have LookupFuncName accept NULL argtypes for 0 args I'm a little tempted to propose that a better solution is to back-patch that patch. Removing the warning alone doesn't make a very strong case for that, but there are other arguments: * Somebody might back-patch code relying on the newer convention; * All else being equal, it's better to keep the code in different branches looking similar. I'm not sure if those arguments justify a back-patch instead of the localized hack you suggest.
Seems like overkill given the age of the surrounding code and the nearness to EOL of releases 11 and 12.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-08-24 Th 16:57, Tom Lane wrote:
>> I'm not sure if those arguments justify a back-patch instead of
>> the localized hack you suggest.
> Seems like overkill given the age of the surrounding code and the
> nearness to EOL of releases 11 and 12.
I agree that 11 is nearly dead, but 12 still has a year-plus to go.
Another point that I wasn't thinking of yesterday is that 11 is
still expected to compile on pre-C99 compilers. I'm not sure
to what extent we are still able to test that -- my old animals
have all gone to buildfarm heaven, although I see that Noah's
AIX menagerie is still soldiering on. Were we relying on "{ 0 }"
anywhere else pre-v12?
regards, tom lane
On 2023-Aug-25, Tom Lane wrote:
> Another point that I wasn't thinking of yesterday is that 11 is
> still expected to compile on pre-C99 compilers. I'm not sure
> to what extent we are still able to test that -- my old animals
> have all gone to buildfarm heaven, although I see that Noah's
> AIX menagerie is still soldiering on. Were we relying on "{ 0 }"
> anywhere else pre-v12?
We have a few occurrences of {0} in initializations in pg11, so it
should work.
$ git grep '{\s*0\s*}' -- *.c
contrib/pgrowlocks/pgrowlocks.c: values[Atnum_xids] = "{0}";
contrib/pgrowlocks/pgrowlocks.c: values[Atnum_pids] = "{0}";
contrib/pgstattuple/pgstatapprox.c: output_type stat = {0};
contrib/pgstattuple/pgstattuple.c: pgstattuple_type stat = {0};
contrib/pgstattuple/pgstattuple.c: pgstattuple_type stat = {0};
src/backend/access/transam/xloginsert.c: XLogRecordBlockCompressHeader cbimg = {0};
src/backend/commands/explain.c: JitInstrumentation ji = {0};
src/backend/commands/explain.c: HashInstrumentation hinstrument = {0};
src/backend/commands/tablecmds.c: static Node bogus_marker = {0}; /* marks conflicting defaults */
src/backend/executor/execExpr.c: ExprEvalStep scratch = {0};
src/backend/executor/execExpr.c: ExprEvalStep scratch = {0};
src/backend/executor/execExpr.c: ExprEvalStep scratch = {0};
src/backend/executor/execExpr.c: ExprEvalStep scratch = {0};
src/backend/executor/execExpr.c: ExprEvalStep scratch = {0};
src/backend/executor/execExpr.c: ExprEvalStep scratch = {0};
src/backend/executor/execExpr.c: ExprEvalStep scratch2 = {0};
src/backend/executor/execExpr.c: ExprEvalStep scratch = {0};
src/backend/executor/execExpr.c: ExprEvalStep scratch = {0};
src/backend/regex/regcomp.c: /* postpone everything else pending possible {0} */
src/backend/regex/regcomp.c: /* annoying special case: {0} or {0,0} cancels everything */
src/backend/utils/adt/jsonfuncs.c: JsValue field = {0};
src/backend/utils/adt/jsonfuncs.c: JsValue jsv = {0};
src/backend/utils/adt/numeric.c:static const NumericDigit const_zero_data[1] = {0};
src/bin/pg_dump/pg_dump.c: "CASE WHEN pol.polroles = '{0}' THEN NULL ELSE "
src/bin/psql/describe.c: " || CASE WHEN polroles <> '{0}' THEN\n"
src/bin/psql/describe.c: " || CASE WHEN polroles <> '{0}' THEN\n"
src/bin/psql/describe.c: " CASE WHEN pol.polroles = '{0}' THEN NULL ELSE
pg_catalog.array_to_string(array(selectrolname from pg_catalog.pg_roles where oid = any (pol.polroles) order by 1),',')
END,\n"
src/interfaces/ecpg/ecpglib/prepare.c:static stmtCacheEntry stmtCacheEntries[16384] = {{0, {0}, 0, 0, 0}};
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Aug-25, Tom Lane wrote:
>> ... Were we relying on "{ 0 }"
>> anywhere else pre-v12?
> We have a few occurrences of {0} in initializations in pg11, so it
> should work.
Ah, indeed. Objection withdrawn then.
regards, tom lane
On 2023-08-26 Sa 17:41, Tom Lane wrote:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:On 2023-Aug-25, Tom Lane wrote:... Were we relying on "{ 0 }" anywhere else pre-v12?We have a few occurrences of {0} in initializations in pg11, so it should work.Ah, indeed. Objection withdrawn then.
OK, thanks, done.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com