pgsql: Cache by-reference missing values in a long lived context

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

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(select rolname 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