code cleanups
Started by Justin Pryzbyabout 3 years ago5 messages
Some modest cleanups I've accumulated.
Attachments:
0001-WIP-do-not-loop-to-set-an-array-to-false.patchtext/x-diff; charset=us-asciiDownload
From 9c5846cc3f04c6797696a234fac0953815e09e4b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 23 Oct 2022 14:52:48 -0500
Subject: [PATCH 1/6] WIP: do not loop to set an array to false
See also:
9fd45870c1436b477264c0c82eb195df52bc0919
572e3e6634e55accf95e2bcfb1340019c86a21dc
---
src/backend/access/transam/xlogprefetcher.c | 5 +---
src/backend/catalog/pg_constraint.c | 11 ++------
src/backend/commands/user.c | 6 +----
src/backend/statistics/extended_stats.c | 6 +----
src/backend/storage/smgr/md.c | 3 +--
src/backend/tcop/pquery.c | 3 +--
src/backend/utils/adt/arrayfuncs.c | 29 ++++++++-------------
src/test/isolation/isolationtester.c | 3 +--
8 files changed, 19 insertions(+), 47 deletions(-)
diff --git a/src/backend/access/transam/xlogprefetcher.c b/src/backend/access/transam/xlogprefetcher.c
index 0cf03945eec..7fcfd146525 100644
--- a/src/backend/access/transam/xlogprefetcher.c
+++ b/src/backend/access/transam/xlogprefetcher.c
@@ -832,13 +832,10 @@ pg_stat_get_recovery_prefetch(PG_FUNCTION_ARGS)
#define PG_STAT_GET_RECOVERY_PREFETCH_COLS 10
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
Datum values[PG_STAT_GET_RECOVERY_PREFETCH_COLS];
- bool nulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS];
+ bool nulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS] = {0};
InitMaterializedSRF(fcinfo, 0);
- for (int i = 0; i < PG_STAT_GET_RECOVERY_PREFETCH_COLS; ++i)
- nulls[i] = false;
-
values[0] = TimestampTzGetDatum(pg_atomic_read_u64(&SharedStats->reset_time));
values[1] = Int64GetDatum(pg_atomic_read_u64(&SharedStats->prefetch));
values[2] = Int64GetDatum(pg_atomic_read_u64(&SharedStats->hit));
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 3a5d4e96439..bd8f32bf62d 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -82,8 +82,8 @@ CreateConstraintEntry(const char *constraintName,
Relation conDesc;
Oid conOid;
HeapTuple tup;
- bool nulls[Natts_pg_constraint];
- Datum values[Natts_pg_constraint];
+ bool nulls[Natts_pg_constraint] = {0};
+ Datum values[Natts_pg_constraint] = {0};
ArrayType *conkeyArray;
ArrayType *confkeyArray;
ArrayType *conpfeqopArray;
@@ -165,13 +165,6 @@ CreateConstraintEntry(const char *constraintName,
else
conexclopArray = NULL;
- /* initialize nulls and values */
- for (i = 0; i < Natts_pg_constraint; i++)
- {
- nulls[i] = false;
- values[i] = (Datum) NULL;
- }
-
conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
Anum_pg_constraint_oid);
values[Anum_pg_constraint_oid - 1] = ObjectIdGetDatum(conOid);
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 8b6543edee2..5af79792136 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1234,8 +1234,7 @@ RenameRole(const char *oldname, const char *newname)
bool isnull;
Datum repl_val[Natts_pg_authid];
bool repl_null[Natts_pg_authid];
- bool repl_repl[Natts_pg_authid];
- int i;
+ bool repl_repl[Natts_pg_authid] = {0};
Oid roleid;
ObjectAddress address;
Form_pg_authid authform;
@@ -1321,9 +1320,6 @@ RenameRole(const char *oldname, const char *newname)
}
/* OK, construct the modified tuple */
- for (i = 0; i < Natts_pg_authid; i++)
- repl_repl[i] = false;
-
repl_repl[Anum_pg_authid_rolname - 1] = true;
repl_val[Anum_pg_authid_rolname - 1] = DirectFunctionCall1(namein,
CStringGetDatum(newname));
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index ab97e71dd79..93a9d3eeafb 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -2343,7 +2343,7 @@ serialize_expr_stats(AnlExprData *exprdata, int nexprs)
VacAttrStats *stats = exprdata[exprno].vacattrstat;
Datum values[Natts_pg_statistic];
- bool nulls[Natts_pg_statistic];
+ bool nulls[Natts_pg_statistic] = {0};
HeapTuple stup;
if (!stats->stats_valid)
@@ -2359,10 +2359,6 @@ serialize_expr_stats(AnlExprData *exprdata, int nexprs)
/*
* Construct a new pg_statistic tuple
*/
- for (i = 0; i < Natts_pg_statistic; ++i)
- {
- nulls[i] = false;
- }
values[Anum_pg_statistic_starelid - 1] = ObjectIdGetDatum(InvalidOid);
values[Anum_pg_statistic_staattnum - 1] = Int16GetDatum(InvalidAttrNumber);
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 14b6fa0fd90..2d5cc97eaa9 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -557,8 +557,7 @@ void
mdopen(SMgrRelation reln)
{
/* mark it not open */
- for (int forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- reln->md_num_open_segs[forknum] = 0;
+ memset(reln->md_num_open_segs, 0, (1+MAX_FORKNUM) * sizeof(*reln->md_num_open_segs));
}
/*
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 52e2db6452b..53b712fd3b5 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -653,8 +653,7 @@ PortalSetResultFormat(Portal portal, int nFormats, int16 *formats)
else
{
/* use default format for all columns */
- for (i = 0; i < natts; i++)
- portal->formats[i] = 0;
+ memset(portal->formats, 0, natts * sizeof(*portal->formats));
}
}
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 495e449a9e9..ff62eb01dfd 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -1038,7 +1038,7 @@ array_out(PG_FUNCTION_ARGS)
i,
j,
k,
- indx[MAXDIM];
+ indx[MAXDIM] = {0};
int ndim,
*dims,
*lb;
@@ -1203,8 +1203,7 @@ array_out(PG_FUNCTION_ARGS)
if (needdims)
APPENDSTR(dims_str);
APPENDCHAR('{');
- for (i = 0; i < ndim; i++)
- indx[i] = 0;
+
j = 0;
k = 0;
do
@@ -4988,10 +4987,9 @@ array_slice_size(char *arraydataptr, bits8 *arraynullsptr,
span[MAXDIM],
prod[MAXDIM],
dist[MAXDIM],
- indx[MAXDIM];
+ indx[MAXDIM] = {0};
char *ptr;
- int i,
- j,
+ int j,
inc;
int count = 0;
@@ -5007,8 +5005,7 @@ array_slice_size(char *arraydataptr, bits8 *arraynullsptr,
typlen, typbyval, typalign);
mda_get_prod(ndim, dim, prod);
mda_get_offset_values(ndim, dist, prod, span);
- for (i = 0; i < ndim; i++)
- indx[i] = 0;
+
j = ndim - 1;
do
{
@@ -5059,9 +5056,8 @@ array_extract_slice(ArrayType *newarray,
prod[MAXDIM],
span[MAXDIM],
dist[MAXDIM],
- indx[MAXDIM];
- int i,
- j,
+ indx[MAXDIM] = {0};
+ int j,
inc;
src_offset = ArrayGetOffset(ndim, dim, lb, st);
@@ -5070,8 +5066,7 @@ array_extract_slice(ArrayType *newarray,
mda_get_prod(ndim, dim, prod);
mda_get_range(ndim, span, st, endp);
mda_get_offset_values(ndim, dist, prod, span);
- for (i = 0; i < ndim; i++)
- indx[i] = 0;
+
dest_offset = 0;
j = ndim - 1;
do
@@ -5138,9 +5133,8 @@ array_insert_slice(ArrayType *destArray,
prod[MAXDIM],
span[MAXDIM],
dist[MAXDIM],
- indx[MAXDIM];
- int i,
- j,
+ indx[MAXDIM] = {0};
+ int j,
inc;
dest_offset = ArrayGetOffset(ndim, dim, lb, st);
@@ -5156,8 +5150,7 @@ array_insert_slice(ArrayType *destArray,
mda_get_prod(ndim, dim, prod);
mda_get_range(ndim, span, st, endp);
mda_get_offset_values(ndim, dist, prod, span);
- for (i = 0; i < ndim; i++)
- indx[i] = 0;
+
src_offset = 0;
j = ndim - 1;
do
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 0a66235153a..19808c59892 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -432,8 +432,7 @@ run_all_permutations(TestSpec *testspec)
* already picked from this pile.
*/
piles = pg_malloc(sizeof(int) * testspec->nsessions);
- for (i = 0; i < testspec->nsessions; i++)
- piles[i] = 0;
+ memset(piles, 0, sizeof(*piles) * testspec->nsessions);
run_all_permutations_recurse(testspec, piles, 0, stepptrs);
--
2.25.1
0002-Avoid-the-most-useless-instances-of-nulls-0-false.patchtext/x-diff; charset=us-asciiDownload
From cc9f9b8b878b4d878b0aa4f04226cb6c2873275e Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 14 Jan 2022 19:25:21 -0600
Subject: [PATCH 2/6] Avoid the most useless instances of nulls[0]=false..
Getting rid of more of these is apparently too controvercial, but these are the
cases where 1) nulls is always being set to false; and, 2) values[] is not
being memset, so there's no parallel between the two.
---
contrib/sslinfo/sslinfo.c | 5 +-
src/backend/replication/logical/origin.c | 13 +---
src/backend/utils/adt/misc.c | 7 +--
src/backend/utils/misc/pg_controldata.c | 80 ++----------------------
4 files changed, 10 insertions(+), 95 deletions(-)
diff --git a/contrib/sslinfo/sslinfo.c b/contrib/sslinfo/sslinfo.c
index 5fd46b98741..d9edfe1b742 100644
--- a/contrib/sslinfo/sslinfo.c
+++ b/contrib/sslinfo/sslinfo.c
@@ -424,7 +424,7 @@ ssl_extension_info(PG_FUNCTION_ARGS)
if (call_cntr < max_calls)
{
Datum values[3];
- bool nulls[3];
+ bool nulls[3] = {0};
char *buf;
HeapTuple tuple;
Datum result;
@@ -453,7 +453,6 @@ ssl_extension_info(PG_FUNCTION_ARGS)
errmsg("unknown OpenSSL extension in certificate at position %d",
call_cntr)));
values[0] = CStringGetTextDatum(OBJ_nid2sn(nid));
- nulls[0] = false;
/* Get the extension value */
if (X509V3_EXT_print(membuf, ext, 0, 0) <= 0)
@@ -463,11 +462,9 @@ ssl_extension_info(PG_FUNCTION_ARGS)
call_cntr)));
len = BIO_get_mem_data(membuf, &buf);
values[1] = PointerGetDatum(cstring_to_text_with_len(buf, len));
- nulls[1] = false;
/* Get critical status */
values[2] = BoolGetDatum(X509_EXTENSION_get_critical(ext));
- nulls[2] = false;
/* Build tuple */
tuple = heap_form_tuple(fctx->tupdesc, values, nulls);
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index f134e44878f..adb3430c33a 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -1527,10 +1527,9 @@ pg_show_replication_origin_status(PG_FUNCTION_ARGS)
continue;
memset(values, 0, sizeof(values));
- memset(nulls, 1, sizeof(nulls));
+ memset(nulls, 0, sizeof(nulls));
values[0] = ObjectIdGetDatum(state->roident);
- nulls[0] = false;
/*
* We're not preventing the origin to be dropped concurrently, so
@@ -1538,19 +1537,13 @@ pg_show_replication_origin_status(PG_FUNCTION_ARGS)
*/
if (replorigin_by_oid(state->roident, true,
&roname))
- {
values[1] = CStringGetTextDatum(roname);
- nulls[1] = false;
- }
+ else
+ nulls[1] = true;
LWLockAcquire(&state->lock, LW_SHARED);
-
values[2] = LSNGetDatum(state->remote_lsn);
- nulls[2] = false;
-
values[3] = LSNGetDatum(state->local_lsn);
- nulls[3] = false;
-
LWLockRelease(&state->lock);
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 9c132512315..611b492624a 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -246,7 +246,7 @@ pg_tablespace_databases(PG_FUNCTION_ARGS)
char *subdir;
bool isempty;
Datum values[1];
- bool nulls[1];
+ bool nulls[1] = {0};
/* this test skips . and .., but is awfully weak */
if (!datOid)
@@ -262,7 +262,6 @@ pg_tablespace_databases(PG_FUNCTION_ARGS)
continue; /* indeed, nothing in it */
values[0] = ObjectIdGetDatum(datOid);
- nulls[0] = false;
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
values, nulls);
@@ -530,11 +529,9 @@ pg_get_catalog_foreign_keys(PG_FUNCTION_ARGS)
{
const SysFKRelationship *fkrel = &sys_fk_relationships[funcctx->call_cntr];
Datum values[6];
- bool nulls[6];
+ bool nulls[6] = {0};
HeapTuple tuple;
- memset(nulls, false, sizeof(nulls));
-
values[0] = ObjectIdGetDatum(fkrel->fk_table);
values[1] = FunctionCall3(arrayinp,
CStringGetDatum(fkrel->fk_columns),
diff --git a/src/backend/utils/misc/pg_controldata.c b/src/backend/utils/misc/pg_controldata.c
index 781f8b87580..32be2b4b5d8 100644
--- a/src/backend/utils/misc/pg_controldata.c
+++ b/src/backend/utils/misc/pg_controldata.c
@@ -32,7 +32,7 @@ Datum
pg_control_system(PG_FUNCTION_ARGS)
{
Datum values[4];
- bool nulls[4];
+ bool nulls[4] = {0};
TupleDesc tupdesc;
HeapTuple htup;
ControlFileData *ControlFile;
@@ -60,16 +60,9 @@ pg_control_system(PG_FUNCTION_ARGS)
(errmsg("calculated CRC checksum does not match value stored in file")));
values[0] = Int32GetDatum(ControlFile->pg_control_version);
- nulls[0] = false;
-
values[1] = Int32GetDatum(ControlFile->catalog_version_no);
- nulls[1] = false;
-
values[2] = Int64GetDatum(ControlFile->system_identifier);
- nulls[2] = false;
-
values[3] = TimestampTzGetDatum(time_t_to_timestamptz(ControlFile->time));
- nulls[3] = false;
htup = heap_form_tuple(tupdesc, values, nulls);
@@ -80,7 +73,7 @@ Datum
pg_control_checkpoint(PG_FUNCTION_ARGS)
{
Datum values[18];
- bool nulls[18];
+ bool nulls[18] = {0};
TupleDesc tupdesc;
HeapTuple htup;
ControlFileData *ControlFile;
@@ -147,60 +140,25 @@ pg_control_checkpoint(PG_FUNCTION_ARGS)
/* Populate the values and null arrays */
values[0] = LSNGetDatum(ControlFile->checkPoint);
- nulls[0] = false;
-
values[1] = LSNGetDatum(ControlFile->checkPointCopy.redo);
- nulls[1] = false;
-
values[2] = CStringGetTextDatum(xlogfilename);
- nulls[2] = false;
-
values[3] = Int32GetDatum(ControlFile->checkPointCopy.ThisTimeLineID);
- nulls[3] = false;
-
values[4] = Int32GetDatum(ControlFile->checkPointCopy.PrevTimeLineID);
- nulls[4] = false;
-
values[5] = BoolGetDatum(ControlFile->checkPointCopy.fullPageWrites);
- nulls[5] = false;
-
values[6] = CStringGetTextDatum(psprintf("%u:%u",
EpochFromFullTransactionId(ControlFile->checkPointCopy.nextXid),
XidFromFullTransactionId(ControlFile->checkPointCopy.nextXid)));
- nulls[6] = false;
-
values[7] = ObjectIdGetDatum(ControlFile->checkPointCopy.nextOid);
- nulls[7] = false;
-
values[8] = TransactionIdGetDatum(ControlFile->checkPointCopy.nextMulti);
- nulls[8] = false;
-
values[9] = TransactionIdGetDatum(ControlFile->checkPointCopy.nextMultiOffset);
- nulls[9] = false;
-
values[10] = TransactionIdGetDatum(ControlFile->checkPointCopy.oldestXid);
- nulls[10] = false;
-
values[11] = ObjectIdGetDatum(ControlFile->checkPointCopy.oldestXidDB);
- nulls[11] = false;
-
values[12] = TransactionIdGetDatum(ControlFile->checkPointCopy.oldestActiveXid);
- nulls[12] = false;
-
values[13] = TransactionIdGetDatum(ControlFile->checkPointCopy.oldestMulti);
- nulls[13] = false;
-
values[14] = ObjectIdGetDatum(ControlFile->checkPointCopy.oldestMultiDB);
- nulls[14] = false;
-
values[15] = TransactionIdGetDatum(ControlFile->checkPointCopy.oldestCommitTsXid);
- nulls[15] = false;
-
values[16] = TransactionIdGetDatum(ControlFile->checkPointCopy.newestCommitTsXid);
- nulls[16] = false;
-
values[17] = TimestampTzGetDatum(time_t_to_timestamptz(ControlFile->checkPointCopy.time));
- nulls[17] = false;
htup = heap_form_tuple(tupdesc, values, nulls);
@@ -211,7 +169,7 @@ Datum
pg_control_recovery(PG_FUNCTION_ARGS)
{
Datum values[5];
- bool nulls[5];
+ bool nulls[5] = {0};
TupleDesc tupdesc;
HeapTuple htup;
ControlFileData *ControlFile;
@@ -241,19 +199,10 @@ pg_control_recovery(PG_FUNCTION_ARGS)
(errmsg("calculated CRC checksum does not match value stored in file")));
values[0] = LSNGetDatum(ControlFile->minRecoveryPoint);
- nulls[0] = false;
-
values[1] = Int32GetDatum(ControlFile->minRecoveryPointTLI);
- nulls[1] = false;
-
values[2] = LSNGetDatum(ControlFile->backupStartPoint);
- nulls[2] = false;
-
values[3] = LSNGetDatum(ControlFile->backupEndPoint);
- nulls[3] = false;
-
values[4] = BoolGetDatum(ControlFile->backupEndRequired);
- nulls[4] = false;
htup = heap_form_tuple(tupdesc, values, nulls);
@@ -264,7 +213,7 @@ Datum
pg_control_init(PG_FUNCTION_ARGS)
{
Datum values[11];
- bool nulls[11];
+ bool nulls[11] = {0};
TupleDesc tupdesc;
HeapTuple htup;
ControlFileData *ControlFile;
@@ -306,37 +255,16 @@ pg_control_init(PG_FUNCTION_ARGS)
(errmsg("calculated CRC checksum does not match value stored in file")));
values[0] = Int32GetDatum(ControlFile->maxAlign);
- nulls[0] = false;
-
values[1] = Int32GetDatum(ControlFile->blcksz);
- nulls[1] = false;
-
values[2] = Int32GetDatum(ControlFile->relseg_size);
- nulls[2] = false;
-
values[3] = Int32GetDatum(ControlFile->xlog_blcksz);
- nulls[3] = false;
-
values[4] = Int32GetDatum(ControlFile->xlog_seg_size);
- nulls[4] = false;
-
values[5] = Int32GetDatum(ControlFile->nameDataLen);
- nulls[5] = false;
-
values[6] = Int32GetDatum(ControlFile->indexMaxKeys);
- nulls[6] = false;
-
values[7] = Int32GetDatum(ControlFile->toast_max_chunk_size);
- nulls[7] = false;
-
values[8] = Int32GetDatum(ControlFile->loblksize);
- nulls[8] = false;
-
values[9] = BoolGetDatum(ControlFile->float8ByVal);
- nulls[9] = false;
-
values[10] = Int32GetDatum(ControlFile->data_checksum_version);
- nulls[10] = false;
htup = heap_form_tuple(tupdesc, values, nulls);
--
2.25.1
0003-selfuncs.c-refactor-parent-ACL-check.patchtext/x-diff; charset=us-asciiDownload
From 54dfbc7d0081de75546003b4c401785127921168 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 25 Sep 2021 18:58:33 -0500
Subject: [PATCH 3/6] selfuncs.c: refactor parent ACL check
The code for statistics expressions was copied from indexes, and this
un-copies it. selfuncs.c is 8k lines long, and this makes it 30 LOC shorter.
See prior discussion:
f97a5902-b4d1-77da-8d2f-6b5b2094fbb7@enterprisedb.com
Tomas said there's similar code elsewhere, and I saw
examine_simple_variable() and ruleutils.c, but I'm unable to find
anything else similar close enough to share.
---
src/backend/utils/adt/selfuncs.c | 140 ++++++++++++-------------------
1 file changed, 52 insertions(+), 88 deletions(-)
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index e0aeaa69092..a9f3ccf9a30 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -188,6 +188,8 @@ static char *convert_string_datum(Datum value, Oid typid, Oid collid,
bool *failure);
static double convert_timevalue_to_scalar(Datum value, Oid typid,
bool *failure);
+static void recheck_parent_acl(PlannerInfo *root, VariableStatData *vardata,
+ Oid relid);
static void examine_simple_variable(PlannerInfo *root, Var *var,
VariableStatData *vardata);
static bool get_variable_range(PlannerInfo *root, VariableStatData *vardata,
@@ -5174,51 +5176,7 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
(pg_class_aclcheck(rte->relid, userid,
ACL_SELECT) == ACLCHECK_OK);
- /*
- * If the user doesn't have permissions to
- * access an inheritance child relation, check
- * the permissions of the table actually
- * mentioned in the query, since most likely
- * the user does have that permission. Note
- * that whole-table select privilege on the
- * parent doesn't quite guarantee that the
- * user could read all columns of the child.
- * But in practice it's unlikely that any
- * interesting security violation could result
- * from allowing access to the expression
- * index's stats, so we allow it anyway. See
- * similar code in examine_simple_variable()
- * for additional comments.
- */
- if (!vardata->acl_ok &&
- root->append_rel_array != NULL)
- {
- AppendRelInfo *appinfo;
- Index varno = index->rel->relid;
-
- appinfo = root->append_rel_array[varno];
- while (appinfo &&
- planner_rt_fetch(appinfo->parent_relid,
- root)->rtekind == RTE_RELATION)
- {
- varno = appinfo->parent_relid;
- appinfo = root->append_rel_array[varno];
- }
- if (varno != index->rel->relid)
- {
- /* Repeat access check on this rel */
- rte = planner_rt_fetch(varno, root);
- Assert(rte->rtekind == RTE_RELATION);
-
- userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
-
- vardata->acl_ok =
- rte->securityQuals == NIL &&
- (pg_class_aclcheck(rte->relid,
- userid,
- ACL_SELECT) == ACLCHECK_OK);
- }
- }
+ recheck_parent_acl(root, vardata, index->rel->relid);
}
else
{
@@ -5308,49 +5266,7 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
(pg_class_aclcheck(rte->relid, userid,
ACL_SELECT) == ACLCHECK_OK);
- /*
- * If the user doesn't have permissions to access an
- * inheritance child relation, check the permissions of
- * the table actually mentioned in the query, since most
- * likely the user does have that permission. Note that
- * whole-table select privilege on the parent doesn't
- * quite guarantee that the user could read all columns of
- * the child. But in practice it's unlikely that any
- * interesting security violation could result from
- * allowing access to the expression stats, so we allow it
- * anyway. See similar code in examine_simple_variable()
- * for additional comments.
- */
- if (!vardata->acl_ok &&
- root->append_rel_array != NULL)
- {
- AppendRelInfo *appinfo;
- Index varno = onerel->relid;
-
- appinfo = root->append_rel_array[varno];
- while (appinfo &&
- planner_rt_fetch(appinfo->parent_relid,
- root)->rtekind == RTE_RELATION)
- {
- varno = appinfo->parent_relid;
- appinfo = root->append_rel_array[varno];
- }
- if (varno != onerel->relid)
- {
- /* Repeat access check on this rel */
- rte = planner_rt_fetch(varno, root);
- Assert(rte->rtekind == RTE_RELATION);
-
- userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
-
- vardata->acl_ok =
- rte->securityQuals == NIL &&
- (pg_class_aclcheck(rte->relid,
- userid,
- ACL_SELECT) == ACLCHECK_OK);
- }
- }
-
+ recheck_parent_acl(root, vardata, onerel->relid);
break;
}
@@ -5360,6 +5276,54 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
}
}
+/*
+ * If the user doesn't have permissions to access an inheritance child
+ * relation, check the permissions of the table actually mentioned in the
+ * query, since most likely the user does have that permission. Note that
+ * whole-table select privilege on the parent doesn't quite guarantee that the
+ * user could read all columns of the child. But in practice it's unlikely
+ * that any interesting security violation could result from allowing access to
+ * the index or expression stats, so we allow it anyway. See similar code in
+ * examine_simple_variable() for additional comments.
+ */
+static void
+recheck_parent_acl(PlannerInfo *root, VariableStatData *vardata, Oid relid)
+{
+ RangeTblEntry *rte;
+ Oid userid;
+
+ if (!vardata->acl_ok &&
+ root->append_rel_array != NULL)
+ {
+ AppendRelInfo *appinfo;
+ Index varno = relid;
+
+ appinfo = root->append_rel_array[varno];
+ while (appinfo &&
+ planner_rt_fetch(appinfo->parent_relid,
+ root)->rtekind == RTE_RELATION)
+ {
+ varno = appinfo->parent_relid;
+ appinfo = root->append_rel_array[varno];
+ }
+
+ if (varno != relid)
+ {
+ /* Repeat access check on this rel */
+ rte = planner_rt_fetch(varno, root);
+ Assert(rte->rtekind == RTE_RELATION);
+
+ userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
+
+ vardata->acl_ok =
+ rte->securityQuals == NIL &&
+ (pg_class_aclcheck(rte->relid,
+ userid,
+ ACL_SELECT) == ACLCHECK_OK);
+ }
+ }
+}
+
/*
* examine_simple_variable
* Handle a simple Var for examine_variable
--
2.25.1
0004-move-beentry-away-from-the-middle-of-the-loop-body.patchtext/x-diff; charset=us-asciiDownload
From 28ffc13ceab70b9e05aa3563b0b8b37e209a349e Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 25 Nov 2021 22:47:41 -0600
Subject: [PATCH 4/6] move beentry++ away from the middle of the loop body
This was more reasonable when the "if" block was shorter; but, now it's
large, and the variable increment is easy to miss, as happened during
patch development:
https://www.postgresql.org/message-id/20211202040653.GM17618@telsasoft.com
---
src/backend/utils/activity/backend_status.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 1146a6c33cd..1cf2bc58410 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -842,7 +842,6 @@ pgstat_read_current_status(void)
CHECK_FOR_INTERRUPTS();
}
- beentry++;
/* Only valid entries get included into the local array */
if (localentry->backendStatus.st_procpid > 0)
{
@@ -869,6 +868,8 @@ pgstat_read_current_status(void)
#endif
localNumBackends++;
}
+
+ beentry++;
}
/* Set the pointer only after completion of a valid table */
--
2.25.1
0005-conjunction-is-cleaner-and-shorter-than-nested-condi.patchtext/x-diff; charset=us-asciiDownload
From 51e25dc00d811965c6e72d1914bd03035d9797fe Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue, 22 Nov 2022 18:44:23 -0600
Subject: [PATCH 5/6] conjunction is cleaner and shorter than nested
conditionals
---
src/backend/replication/pgoutput/pgoutput.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 2ecaa5b9074..2ec201e3007 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -810,7 +810,7 @@ pgoutput_row_filter_exec_expr(ExprState *state, ExprContext *econtext)
ret = ExecEvalExprSwitchContext(state, econtext, &isnull);
elog(DEBUG3, "row filter evaluates to %s (isnull: %s)",
- isnull ? "false" : DatumGetBool(ret) ? "true" : "false",
+ !isnull && DatumGetBool(ret) ? "true" : "false",
isnull ? "true" : "false");
if (isnull)
--
2.25.1
0006-basebackup-use-port-strlcpy.patchtext/x-diff; charset=us-asciiDownload
From f3c4c2cf735c448edd66e9f0f546547355aff1c1 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 13 Jul 2022 15:06:21 -0500
Subject: [PATCH 6/6] basebackup: use port/strlcpy()
---
src/bin/pg_basebackup/pg_basebackup.c | 3 +--
src/bin/pg_basebackup/pg_receivewal.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 22836ca01a1..28960158e7f 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1005,8 +1005,7 @@ parse_compress_options(char *option, char **algorithm, char **detail,
char *alg;
alg = palloc((sep - option) + 1);
- memcpy(alg, option, sep - option);
- alg[sep - option] = '\0';
+ strlcpy(alg, option, sep - option + 1);
*algorithm = alg;
*detail = pstrdup(sep + 1);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 63207ca025e..3acf19a1201 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -161,8 +161,7 @@ parse_compress_options(char *option, char **algorithm, char **detail)
char *alg;
alg = palloc((sep - option) + 1);
- memcpy(alg, option, sep - option);
- alg[sep - option] = '\0';
+ strlcpy(alg, option, sep - option + 1);
*algorithm = alg;
*detail = pstrdup(sep + 1);
--
2.25.1
Re: code cleanups
Justin Pryzby <pryzby@telsasoft.com> writes:
Some modest cleanups I've accumulated.
Hmm ...
I don't especially care for either 0001 or 0002, mainly because
I do not agree that this is good style:
- bool nulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS];
+ bool nulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS] = {0};It causes the code to be far more in bed than I like with the assumption
that we're initializing to physical zeroes. The explicit loop method
can be trivially adjusted to initialize to "true" or some other value;
at least for bool arrays, that's true of memset'ing as well. But this,
if you decide you need something other than zeroes, is a foot-gun.
In particular, someone whose C is a bit weak might mistakenly think that
bool nulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS] = {true};
will set all the array elements to true. Nor is there a plausible
argument that this is more efficient. So I don't care for this approach
and I don't want to adopt it.
0003: I agree with getting rid of the duplicated code, but did you go
far enough? Isn't the code just above those parent checks also pretty
redundant? It would be more intellectually consistent to move the full
responsibility for setting acl_ok into a subroutine. This shows in
the patch as you have it because the header comment for
recheck_parent_acl is completely out-of-context.
0004: Right, somebody injected code in a poorly chosen place
(yet another victim of the "add at the end" anti-pattern).
0005: No particular objection, but it's not much of an improvement
either. It seems maybe a shade less consistent with the following
line.
0006: These changes will cause fetching of one more source byte than
was fetched before. I'm not sure that's safe, so I don't think this
is an improvement.
regards, tom lane
Re: code cleanups
On Wed, Nov 23, 2022 at 12:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
at least for bool arrays, that's true of memset'ing as well. But this,
if you decide you need something other than zeroes, is a foot-gun.
In particular, someone whose C is a bit weak might mistakenly think thatbool nulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS] = {true};
will set all the array elements to true. Nor is there a plausible
argument that this is more efficient. So I don't care for this approach
and I don't want to adopt it.
I don't really know what the argument is for the explicit initializer
style, but I think this argument against it is pretty weak.
It should be more than fine to assume that anyone who is hacking on
PostgreSQL is proficient in C. It's true that there might be some
people who aren't, or who aren't familiar with the limitations of the
initializer construct, and I include myself in that latter category. I
don't think it was part of C when I learned C. But if we don't possess
the collective expertise as a project to bring people who have missed
these details of the C programming language up to speed, we should
just throw in the towel now and go home.
Hacking on PostgreSQL is HARD and it relies on knowing FAR more than
just the basics of how to code in C. Put differently, if you can't
even figure out how C works, you have no chance of doing anything very
interesting with the PostgreSQL code base, because you're going to
have to figure out a lot more than the basics of the implementation
language to make a meaningful contribution.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: code cleanups
On Thu, Nov 24, 2022 at 12:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Justin Pryzby <pryzby@telsasoft.com> writes:
Some modest cleanups I've accumulated.
0004: Right, somebody injected code in a poorly chosen place
(yet another victim of the "add at the end" anti-pattern).
I've pushed 0004.
--
John Naylor
EDB: http://www.enterprisedb.com
Re: code cleanups
Justin Pryzby <pryzby(at)telsasoft(dot)com> writes:
Some modest cleanups I've accumulated
Hi Justin.
0001:
Regarding initializer {0}, the problem is still with old compilers, which
don't initialize exactly like memset.
Only more modern compilers fill in any "holes" that may exist.
This means that as old compilers are not supported, this will no longer be
a problem.
Fast and secure solution: memset
+1 to switch from loop to memset, same as many places in the code base.
- /* initialize nulls and values */
- for (i = 0; i < Natts_pg_constraint; i++)
- {
- nulls[i] = false;
- values[i] = (Datum) NULL;
- }
+ memset(nulls, false, sizeof(nulls));
+ memset(values, 0, sizeof(values));What made me curious about this excerpt is that the Datum values are NULL,
but aren't nulls?
Would it not be?
+ memset(nulls, true, sizeof(nulls));
+ memset(values, 0, sizeof(values));
src/backend/tcop/pquery.c:
/* single format specified, use for all columns */
-int16 format1 = formats[0];
-
-for (i = 0; i < natts; i++)
-portal->formats[i] = format1;
+ memset(portal->formats, formats[0], natts * sizeof(*portal->formats));0002:
contrib/sslinfo/sslinfo.c
memset is faster than intercalated stores.
src/backend/replication/logical/origin.c
+1
one store, is better than three.
but, should be:
- memset(nulls, 1, sizeof(nulls));
+memset(nulls, false, sizeof(nulls));The correct style is false, not 0.
src/backend/utils/adt/misc.c:
-1
It got worse.
It's only one store, which could be avoided by the "continue" statement.
src/backend/utils/misc/pg_controldata.c:
+1
+memset(nulls, false, sizeof(nulls));or
nulls[0] = false;
nulls[1] = false;
nulls[2] = false;
nulls[3] = false;
Bad style, intercalated stores are worse.
0003:
+1
But you should reduce the scope of vars:
RangeTblEntry *rte
Oid userid;
+ if (varno != relid)
+ {
+ RangeTblEntry *rte;
+ Oid userid;0005:
+1
0006:
+1
regards,
Ranier Vilela
Import Notes
Resolved by subject fallback