Re: Remove unnecessary commas for goto labels
On Sun, 09 Oct 2022 at 11:07, Julien Rouhaud wrote: > Hi, > > On Sun, Oct 09, 2022 at 07:42:58AM +0800, Japin Li wrote: >> >> Hi hackers, >> >> I find there are some unnecessary commas for goto lables, >> attached a patch to remove them. > > You mean semi-colon? +1, and a quick regex later I don't see any other > occurrence. Yeah semi-colon, sorry for the typo. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Remove unnecessary commas for goto labels
Hi, On Sun, Oct 09, 2022 at 07:42:58AM +0800, Japin Li wrote: > > Hi hackers, > > I find there are some unnecessary commas for goto lables, > attached a patch to remove them. You mean semi-colon? +1, and a quick regex later I don't see any other occurrence.
Re: START_REPLICATION SLOT causing a crash in an assert build
On 10/8/22 1:40 PM, Andres Freund wrote: On 2022-10-08 09:53:50 -0700, Andres Freund wrote: On 2022-10-07 19:56:33 -0700, Andres Freund wrote: I'm planning to push this either later tonight (if I feel up to it after cooking dinner) or tomorrow morning PST, due to the release wrap deadline. I looked this over again, tested a bit more, and pushed the adjusted 15 and master versions to github to get a CI run. Once that passes, as I expect, I'll push them for real. Those passed and thus pushed. Thanks for the report, debugging and review everyone! Thanks for the quick turnaround! I've closed the open item. Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: use has_privs_of_role() for pg_hba.conf
On Sat, Oct 08, 2022 at 10:12:22AM -0700, Nathan Bossart wrote: > Okay, I think there are sufficient votes against this change to simply mark > it Rejected. Thanks for the discussion! Even if the patch is at the end rejected, I think that the test is still useful once you switch its logic to use membership and not inherited privileges for the roles created, and there is zero coverage for "samplegroup" and its kind currently. -- Michael signature.asc Description: PGP signature
Remove unnecessary commas for goto labels
Hi hackers, I find there are some unnecessary commas for goto lables, attached a patch to remove them. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd. >From 3ac7d03e1c5d81e883406ac62fafbe7e7d32fd1e Mon Sep 17 00:00:00 2001 From: Japin Li Date: Sun, 9 Oct 2022 07:35:43 +0800 Subject: [PATCH v1 1/1] Remove unnecessary commas for goto labels --- src/backend/access/transam/slru.c | 2 +- src/backend/executor/nodeModifyTable.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c index b65cb49d7f..6c57fae312 100644 --- a/src/backend/access/transam/slru.c +++ b/src/backend/access/transam/slru.c @@ -1239,7 +1239,7 @@ SimpleLruTruncate(SlruCtl ctl, int cutoffPage) */ LWLockAcquire(shared->ControlLock, LW_EXCLUSIVE); -restart:; +restart: /* * While we are holding the lock, make an important safety check: the diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 04454ad6e6..447f7bc2fb 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1373,7 +1373,7 @@ ExecDelete(ModifyTableContext *context, * special-case behavior needed for referential integrity updates in * transaction-snapshot mode transactions. */ -ldelete:; +ldelete: result = ExecDeleteAct(context, resultRelInfo, tupleid, changingPart); switch (result) @@ -1855,7 +1855,7 @@ ExecUpdateAct(ModifyTableContext *context, ResultRelInfo *resultRelInfo, * then trigger.c will have done table_tuple_lock to lock the correct * tuple, so there's no need to do them again.) */ -lreplace:; +lreplace: /* ensure slot is independent, consider e.g. EPQ */ ExecMaterializeSlot(slot); @@ -2686,7 +2686,7 @@ ExecMergeMatched(ModifyTableContext *context, ResultRelInfo *resultRelInfo, econtext->ecxt_innertuple = context->planSlot; econtext->ecxt_outertuple = NULL; -lmerge_matched:; +lmerge_matched: /* * This routine is only invoked for matched rows, and we must have found -- 2.17.1
Re: Amcheck verification of GiST and GIN
On Sun, Oct 2, 2022 at 12:12 AM Andres Freund wrote: > > Here's an updated patch adding meson compat. Thank you, Andres! Here's one more rebase (something was adjusted in amcheck build). Also I've fixed new warnings except warning about absent heapallindexed for GIN. It's a TODO. Thanks! Best regards, Andrey Borodin. v15-0002-Add-gist_index_parent_check-function-to-verify-G.patch Description: Binary data v15-0003-Add-gin_index_parent_check-to-verify-GIN-index.patch Description: Binary data v15-0001-Refactor-amcheck-to-extract-common-locking-routi.patch Description: Binary data
Re: Switching XLog source from archive to streaming when primary available
On Mon, Sep 19, 2022 at 07:49:21PM +0530, Bharath Rupireddy wrote: > SwitchFromArchiveToStreamEnabled() seemed better at this point. I'm > attaching the v7 patch with that change. Please review it further. As I mentioned upthread [0], I'm still a little concerned that this patch will cause the state machine to go straight from archive recovery to streaming replication, skipping recovery from pg_wal. I wonder if this could be resolved by moving the standby to the pg_wal phase instead. Concretely, this line + if (switchSource) + break; would instead change currentSource from XLOG_FROM_ARCHIVE to XLOG_FROM_PG_WAL before the call to XLogFileReadAnyTLI(). I suspect the behavior would be basically the same, but it would maintain the existing ordering. However, I do see the following note elsewhere in xlogrecovery.c: * The segment can be fetched via restore_command, or via walreceiver having * streamed the record, or it can already be present in pg_wal. Checking * pg_wal is mainly for crash recovery, but it will be polled in standby mode * too, in case someone copies a new segment directly to pg_wal. That is not * documented or recommended, though. Given this information, the present behavior might not be too important, but I don't see a point in changing it without good reason. [0] https://postgr.es/m/20220906215704.GA2084086%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Adding Support for Copy callback functionality on COPY TO api
On Sat, Oct 08, 2022 at 10:37:41AM -0700, Nathan Bossart wrote:
> Yeah, that makes more sense. It actually simplifies things a bit, too.
Sorry for the noise. There was an extra #include in v4 that I've removed
in v5.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 6e80d41135b8b21f9b06e09a7e85069acc8e57a8 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 2 Aug 2022 16:15:01 -0700
Subject: [PATCH v5 1/1] Support COPY TO callback functions.
---
src/backend/commands/copy.c | 2 +-
src/backend/commands/copyto.c | 18 ++--
src/include/commands/copy.h | 3 +-
src/test/modules/Makefile | 1 +
src/test/modules/meson.build | 1 +
.../modules/test_copy_callbacks/.gitignore| 4 ++
src/test/modules/test_copy_callbacks/Makefile | 23 ++
.../expected/test_copy_callbacks.out | 12 +
.../modules/test_copy_callbacks/meson.build | 34 ++
.../sql/test_copy_callbacks.sql | 4 ++
.../test_copy_callbacks--1.0.sql | 8
.../test_copy_callbacks/test_copy_callbacks.c | 46 +++
.../test_copy_callbacks.control | 4 ++
src/tools/pgindent/typedefs.list | 1 +
14 files changed, 156 insertions(+), 5 deletions(-)
create mode 100644 src/test/modules/test_copy_callbacks/.gitignore
create mode 100644 src/test/modules/test_copy_callbacks/Makefile
create mode 100644 src/test/modules/test_copy_callbacks/expected/test_copy_callbacks.out
create mode 100644 src/test/modules/test_copy_callbacks/meson.build
create mode 100644 src/test/modules/test_copy_callbacks/sql/test_copy_callbacks.sql
create mode 100644 src/test/modules/test_copy_callbacks/test_copy_callbacks--1.0.sql
create mode 100644 src/test/modules/test_copy_callbacks/test_copy_callbacks.c
create mode 100644 src/test/modules/test_copy_callbacks/test_copy_callbacks.control
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 49924e476a..db4c9dbc23 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -310,7 +310,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
cstate = BeginCopyTo(pstate, rel, query, relid,
stmt->filename, stmt->is_program,
- stmt->attlist, stmt->options);
+ NULL, stmt->attlist, stmt->options);
*processed = DoCopyTo(cstate); /* copy from database to file */
EndCopyTo(cstate);
}
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index fca29a9a10..a7b8ec030d 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -51,6 +51,7 @@ typedef enum CopyDest
{
COPY_FILE, /* to file (or a piped program) */
COPY_FRONTEND,/* to frontend */
+ COPY_CALLBACK,/* to callback function */
} CopyDest;
/*
@@ -85,6 +86,7 @@ typedef struct CopyToStateData
List *attnumlist; /* integer list of attnums to copy */
char *filename; /* filename, or NULL for STDOUT */
bool is_program; /* is 'filename' a program to popen? */
+ copy_data_dest_cb data_dest_cb; /* function for writing data */
CopyFormatOptions opts;
Node *whereClause; /* WHERE condition (or NULL) */
@@ -247,6 +249,9 @@ CopySendEndOfRow(CopyToState cstate)
/* Dump the accumulated row as one CopyData message */
(void) pq_putmessage('d', fe_msgbuf->data, fe_msgbuf->len);
break;
+ case COPY_CALLBACK:
+ cstate->data_dest_cb(fe_msgbuf->data, fe_msgbuf->len);
+ break;
}
/* Update the progress */
@@ -344,11 +349,12 @@ BeginCopyTo(ParseState *pstate,
Oid queryRelId,
const char *filename,
bool is_program,
+ copy_data_dest_cb data_dest_cb,
List *attnamelist,
List *options)
{
CopyToState cstate;
- bool pipe = (filename == NULL);
+ bool pipe = (filename == NULL && data_dest_cb == NULL);
TupleDesc tupDesc;
int num_phys_attrs;
MemoryContext oldcontext;
@@ -656,7 +662,13 @@ BeginCopyTo(ParseState *pstate,
cstate->copy_dest = COPY_FILE; /* default */
- if (pipe)
+ if (data_dest_cb)
+ {
+ progress_vals[1] = PROGRESS_COPY_TYPE_CALLBACK;
+ cstate->copy_dest = COPY_CALLBACK;
+ cstate->data_dest_cb = data_dest_cb;
+ }
+ else if (pipe)
{
progress_vals[1] = PROGRESS_COPY_TYPE_PIPE;
@@ -769,7 +781,7 @@ EndCopyTo(CopyToState cstate)
uint64
DoCopyTo(CopyToState cstate)
{
- bool pipe = (cstate->filename == NULL);
+ bool pipe = (cstate->filename == NULL && cstate->data_dest_cb == NULL);
bool fe_copy = (pipe && whereToSendOutput == DestRemote);
TupleDesc tupDesc;
int num_phys_attrs;
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 3f6677b132..b77b935005 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -66,6 +66,7 @@ typedef struct CopyFromStateData *CopyFromState;
typedef struct CopyToStateData *CopyToState;
typedef int (*copy_data_source_cb) (void *outbuf, int minread, int maxread);
Re: ICU for global collation
On 2022-10-01 15:07, Peter Eisentraut wrote:
On 22.09.22 20:06, Marina Polyakova wrote:
On 2022-09-21 17:53, Peter Eisentraut wrote:
Committed with that test, thanks. I think that covers all the ICU
issues you reported for PG15 for now?
I thought about the order of the ICU checks - if it is ok to check
that the selected encoding is supported by ICU after printing all the
locale & encoding information, why not to move almost all the ICU
checks here?..
It's possible that we can do better, but I'm not going to add things
like that to PG 15 at this point unless it fixes a faulty behavior.
Will PG 15 always have this order of ICU checks, is the current
behaviour correct enough? On the other hand, there may be a better fix
for PG 16+ and not all changes can be backported...
On 2022-09-16 10:56, Peter Eisentraut wrote:
On 15.09.22 17:41, Marina Polyakova wrote:
I agree with you. Here's another version of the patch. The
locale/encoding checks and reports in initdb have been reordered,
because now the encoding is set first and only then the ICU locale is
checked.
I committed something based on the first version of your patch. This
reordering of the messages here was a little too much surgery for me
at this point. For instance, there are also messages in #ifdef WIN32
code that would need to be reordered as well. I kept the overall
structure of the code the same and just inserted the additional
proposed checks.
If you want to pursue the reordering of the checks and messages
overall, a patch for the master branch could be considered.
I've worked on this again (see attached patch) but I'm not sure if the
messages of encoding mismatches are clear enough without the full locale
information. For
$ initdb -D data --icu-locale en --locale-provider icu
compare the outputs:
The database cluster will be initialized with this locale configuration:
provider:icu
ICU locale: en
LC_COLLATE: de_DE.iso885915@euro
LC_CTYPE:de_DE.iso885915@euro
LC_MESSAGES: en_US.utf8
LC_MONETARY: de_DE.iso885915@euro
LC_NUMERIC: de_DE.iso885915@euro
LC_TIME: de_DE.iso885915@euro
The default database encoding has been set to "UTF8".
initdb: error: encoding mismatch
initdb: detail: The encoding you selected (UTF8) and the encoding that
the selected locale uses (LATIN9) do not match. This would lead to
misbehavior in various character string processing functions.
initdb: hint: Rerun initdb and either do not specify an encoding
explicitly, or choose a matching combination.
and
Encoding "UTF8" implied by locale will be set as the default database
encoding.
initdb: error: encoding mismatch
initdb: detail: The encoding you selected (UTF8) and the encoding that
the selected locale uses (LATIN9) do not match. This would lead to
misbehavior in various character string processing functions.
initdb: hint: Rerun initdb and either do not specify an encoding
explicitly, or choose a matching combination.
The same without ICU, e.g. for
$ initdb -D data
the output with locale information:
The database cluster will be initialized with this locale configuration:
provider:libc
LC_COLLATE: en_US.utf8
LC_CTYPE:de_DE.iso885915@euro
LC_MESSAGES: en_US.utf8
LC_MONETARY: de_DE.iso885915@euro
LC_NUMERIC: de_DE.iso885915@euro
LC_TIME: de_DE.iso885915@euro
The default database encoding has accordingly been set to "LATIN9".
initdb: error: encoding mismatch
initdb: detail: The encoding you selected (LATIN9) and the encoding that
the selected locale uses (UTF8) do not match. This would lead to
misbehavior in various character string processing functions.
initdb: hint: Rerun initdb and either do not specify an encoding
explicitly, or choose a matching combination.
and the "shorter" version:
Encoding "LATIN9" implied by locale will be set as the default database
encoding.
initdb: error: encoding mismatch
initdb: detail: The encoding you selected (LATIN9) and the encoding that
the selected locale uses (UTF8) do not match. This would lead to
misbehavior in various character string processing functions.
initdb: hint: Rerun initdb and either do not specify an encoding
explicitly, or choose a matching combination.
BTW, what did you mean that "there are also messages in #ifdef WIN32
code that would need to be reordered as well"?..
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 96b46cbc020ef8b85b6d54d3d4ca8ad116277832..242d68f58287aeb6f95619c2ce8b78e38433cf18 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -1030,6 +1030,11 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
if (dblocprovider == COLLPROVIDER_ICU)
{
+#ifndef USE_ICU
+ ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ICU is not supported in this build")));
+#else
if
Re: Non-robustness in pmsignal.c
Andres Freund writes: > When can PostmasterContext be NULL here, and why can we just continue without > (re-)allocating PMChildInUse? We'd only get into the !found stanza in a postmaster or a standalone backend. A standalone backend isn't ever going to call AssignPostmasterChildSlot or ReleasePostmasterChildSlot, so it does not need the array; and it also doesn't have a PostmasterContext, so there's not a good place to allocate the array either. Perhaps there's a better way to distinguish am-I-a-postmaster, but I thought checking PostmasterContext is fine since that ties directly to what the code needs to do. Yes, the code would malfunction if the PostmasterContext != NULL condition changed from one cycle to the next, but that shouldn't happen. regards, tom lane
Re: START_REPLICATION SLOT causing a crash in an assert build
On 2022-10-08 09:53:50 -0700, Andres Freund wrote: > On 2022-10-07 19:56:33 -0700, Andres Freund wrote: > > I'm planning to push this either later tonight (if I feel up to it after > > cooking dinner) or tomorrow morning PST, due to the release wrap deadline. > > I looked this over again, tested a bit more, and pushed the adjusted 15 and > master versions to github to get a CI run. Once that passes, as I expect, I'll > push them for real. Those passed and thus pushed. Thanks for the report, debugging and review everyone! I think we need at least the following tests for replslots: - a reset while decoding is ongoing works correctly - replslot stats continue to be accumulated after stats have been removed I wonder how much it'd take to teach isolationtester to handle the replication protocol...
Re: Adding Support for Copy callback functionality on COPY TO api
On Sat, Oct 08, 2022 at 02:11:38PM +0900, Michael Paquier wrote:
> Using an ereport(NOTICE) to show the data reported in the callback is
> fine by me. How about making the module a bit more modular, by
> passing as argument a regclass and building a list of arguments with
> it? You may want to hold the ShareAccessLock on the relation until
> the end of the transaction in this example.
Yeah, that makes more sense. It actually simplifies things a bit, too.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 0b45607988e28b405c7795de0c7f82f51d4662e5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 2 Aug 2022 16:15:01 -0700
Subject: [PATCH v4 1/1] Support COPY TO callback functions.
---
src/backend/commands/copy.c | 2 +-
src/backend/commands/copyto.c | 18 +--
src/include/commands/copy.h | 3 +-
src/test/modules/Makefile | 1 +
src/test/modules/meson.build | 1 +
.../modules/test_copy_callbacks/.gitignore| 4 ++
src/test/modules/test_copy_callbacks/Makefile | 23 +
.../expected/test_copy_callbacks.out | 12 +
.../modules/test_copy_callbacks/meson.build | 34 ++
.../sql/test_copy_callbacks.sql | 4 ++
.../test_copy_callbacks--1.0.sql | 8
.../test_copy_callbacks/test_copy_callbacks.c | 47 +++
.../test_copy_callbacks.control | 4 ++
src/tools/pgindent/typedefs.list | 1 +
14 files changed, 157 insertions(+), 5 deletions(-)
create mode 100644 src/test/modules/test_copy_callbacks/.gitignore
create mode 100644 src/test/modules/test_copy_callbacks/Makefile
create mode 100644 src/test/modules/test_copy_callbacks/expected/test_copy_callbacks.out
create mode 100644 src/test/modules/test_copy_callbacks/meson.build
create mode 100644 src/test/modules/test_copy_callbacks/sql/test_copy_callbacks.sql
create mode 100644 src/test/modules/test_copy_callbacks/test_copy_callbacks--1.0.sql
create mode 100644 src/test/modules/test_copy_callbacks/test_copy_callbacks.c
create mode 100644 src/test/modules/test_copy_callbacks/test_copy_callbacks.control
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 49924e476a..db4c9dbc23 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -310,7 +310,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
cstate = BeginCopyTo(pstate, rel, query, relid,
stmt->filename, stmt->is_program,
- stmt->attlist, stmt->options);
+ NULL, stmt->attlist, stmt->options);
*processed = DoCopyTo(cstate); /* copy from database to file */
EndCopyTo(cstate);
}
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index fca29a9a10..a7b8ec030d 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -51,6 +51,7 @@ typedef enum CopyDest
{
COPY_FILE, /* to file (or a piped program) */
COPY_FRONTEND,/* to frontend */
+ COPY_CALLBACK,/* to callback function */
} CopyDest;
/*
@@ -85,6 +86,7 @@ typedef struct CopyToStateData
List *attnumlist; /* integer list of attnums to copy */
char *filename; /* filename, or NULL for STDOUT */
bool is_program; /* is 'filename' a program to popen? */
+ copy_data_dest_cb data_dest_cb; /* function for writing data */
CopyFormatOptions opts;
Node *whereClause; /* WHERE condition (or NULL) */
@@ -247,6 +249,9 @@ CopySendEndOfRow(CopyToState cstate)
/* Dump the accumulated row as one CopyData message */
(void) pq_putmessage('d', fe_msgbuf->data, fe_msgbuf->len);
break;
+ case COPY_CALLBACK:
+ cstate->data_dest_cb(fe_msgbuf->data, fe_msgbuf->len);
+ break;
}
/* Update the progress */
@@ -344,11 +349,12 @@ BeginCopyTo(ParseState *pstate,
Oid queryRelId,
const char *filename,
bool is_program,
+ copy_data_dest_cb data_dest_cb,
List *attnamelist,
List *options)
{
CopyToState cstate;
- bool pipe = (filename == NULL);
+ bool pipe = (filename == NULL && data_dest_cb == NULL);
TupleDesc tupDesc;
int num_phys_attrs;
MemoryContext oldcontext;
@@ -656,7 +662,13 @@ BeginCopyTo(ParseState *pstate,
cstate->copy_dest = COPY_FILE; /* default */
- if (pipe)
+ if (data_dest_cb)
+ {
+ progress_vals[1] = PROGRESS_COPY_TYPE_CALLBACK;
+ cstate->copy_dest = COPY_CALLBACK;
+ cstate->data_dest_cb = data_dest_cb;
+ }
+ else if (pipe)
{
progress_vals[1] = PROGRESS_COPY_TYPE_PIPE;
@@ -769,7 +781,7 @@ EndCopyTo(CopyToState cstate)
uint64
DoCopyTo(CopyToState cstate)
{
- bool pipe = (cstate->filename == NULL);
+ bool pipe = (cstate->filename == NULL && cstate->data_dest_cb == NULL);
bool fe_copy = (pipe && whereToSendOutput == DestRemote);
TupleDesc tupDesc;
int num_phys_attrs;
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 3f6677b132..b77b935005 100644
---
Re: Non-robustness in pmsignal.c
Hi,
On 2022-10-08 13:15:07 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund writes:
> >> Only PM_CHILD_ACTIVE and PM_CHILD_WALSENDER though. We could afford another
> >> MaxLivePostmasterChildren() sized array...
>
> > Oh, I see what you mean --- one private and one public array.
> > Maybe that makes more sense than what I did, not sure.
>
> Yeah, that's definitely a better way. I'll push this after the
> release freeze lifts.
Cool, thanks for exploring.
> /*
> * Signal handler to be notified if postmaster dies.
> */
> @@ -142,7 +152,25 @@ PMSignalShmemInit(void)
> {
> /* initialize all flags to zeroes */
> MemSet(unvolatize(PMSignalData *, PMSignalState), 0,
> PMSignalShmemSize());
> - PMSignalState->num_child_flags = MaxLivePostmasterChildren();
> + num_child_inuse = MaxLivePostmasterChildren();
> + PMSignalState->num_child_flags = num_child_inuse;
> +
> + /*
> + * Also allocate postmaster's private PMChildInUse[] array. We
> + * might've already done that in a previous shared-memory
> creation
> + * cycle, in which case free the old array to avoid a leak.
> (Do it
> + * like this to support the possibility that
> MaxLivePostmasterChildren
> + * changed.) In a standalone backend, we do not need this.
> + */
> + if (PostmasterContext != NULL)
> + {
> + if (PMChildInUse)
> + pfree(PMChildInUse);
> + PMChildInUse = (bool *)
> + MemoryContextAllocZero(PostmasterContext,
> +
> num_child_inuse * sizeof(bool));
> + }
> + next_child_inuse = 0;
> }
> }
When can PostmasterContext be NULL here, and why can we just continue without
(re-)allocating PMChildInUse?
Greetings,
Andres Freund
Re: Non-robustness in pmsignal.c
I wrote:
> Andres Freund writes:
>> Only PM_CHILD_ACTIVE and PM_CHILD_WALSENDER though. We could afford another
>> MaxLivePostmasterChildren() sized array...
> Oh, I see what you mean --- one private and one public array.
> Maybe that makes more sense than what I did, not sure.
Yeah, that's definitely a better way. I'll push this after the
release freeze lifts.
regards, tom lane
diff --git a/src/backend/storage/ipc/pmsignal.c b/src/backend/storage/ipc/pmsignal.c
index 3f0ec5e6b8..c85521d364 100644
--- a/src/backend/storage/ipc/pmsignal.c
+++ b/src/backend/storage/ipc/pmsignal.c
@@ -26,6 +26,7 @@
#include "replication/walsender.h"
#include "storage/pmsignal.h"
#include "storage/shmem.h"
+#include "utils/memutils.h"
/*
@@ -75,12 +76,21 @@ struct PMSignalData
QuitSignalReason sigquit_reason; /* why SIGQUIT was sent */
/* per-child-process flags */
int num_child_flags; /* # of entries in PMChildFlags[] */
- int next_child_flag; /* next slot to try to assign */
sig_atomic_t PMChildFlags[FLEXIBLE_ARRAY_MEMBER];
};
+/* PMSignalState pointer is valid in both postmaster and child processes */
NON_EXEC_STATIC volatile PMSignalData *PMSignalState = NULL;
+/*
+ * These static variables are valid only in the postmaster. We keep a
+ * duplicative private array so that we can trust its state even if some
+ * failing child has clobbered the PMSignalData struct in shared memory.
+ */
+static int num_child_inuse; /* # of entries in PMChildInUse[] */
+static int next_child_inuse; /* next slot to try to assign */
+static bool *PMChildInUse; /* true if i'th flag slot is assigned */
+
/*
* Signal handler to be notified if postmaster dies.
*/
@@ -142,7 +152,25 @@ PMSignalShmemInit(void)
{
/* initialize all flags to zeroes */
MemSet(unvolatize(PMSignalData *, PMSignalState), 0, PMSignalShmemSize());
- PMSignalState->num_child_flags = MaxLivePostmasterChildren();
+ num_child_inuse = MaxLivePostmasterChildren();
+ PMSignalState->num_child_flags = num_child_inuse;
+
+ /*
+ * Also allocate postmaster's private PMChildInUse[] array. We
+ * might've already done that in a previous shared-memory creation
+ * cycle, in which case free the old array to avoid a leak. (Do it
+ * like this to support the possibility that MaxLivePostmasterChildren
+ * changed.) In a standalone backend, we do not need this.
+ */
+ if (PostmasterContext != NULL)
+ {
+ if (PMChildInUse)
+pfree(PMChildInUse);
+ PMChildInUse = (bool *)
+MemoryContextAllocZero(PostmasterContext,
+ num_child_inuse * sizeof(bool));
+ }
+ next_child_inuse = 0;
}
}
@@ -218,21 +246,24 @@ GetQuitSignalReason(void)
int
AssignPostmasterChildSlot(void)
{
- int slot = PMSignalState->next_child_flag;
+ int slot = next_child_inuse;
int n;
/*
- * Scan for a free slot. We track the last slot assigned so as not to
- * waste time repeatedly rescanning low-numbered slots.
+ * Scan for a free slot. Notice that we trust nothing about the contents
+ * of PMSignalState, but use only postmaster-local data for this decision.
+ * We track the last slot assigned so as not to waste time repeatedly
+ * rescanning low-numbered slots.
*/
- for (n = PMSignalState->num_child_flags; n > 0; n--)
+ for (n = num_child_inuse; n > 0; n--)
{
if (--slot < 0)
- slot = PMSignalState->num_child_flags - 1;
- if (PMSignalState->PMChildFlags[slot] == PM_CHILD_UNUSED)
+ slot = num_child_inuse - 1;
+ if (!PMChildInUse[slot])
{
+ PMChildInUse[slot] = true;
PMSignalState->PMChildFlags[slot] = PM_CHILD_ASSIGNED;
- PMSignalState->next_child_flag = slot;
+ next_child_inuse = slot;
return slot + 1;
}
}
@@ -254,7 +285,7 @@ ReleasePostmasterChildSlot(int slot)
{
bool result;
- Assert(slot > 0 && slot <= PMSignalState->num_child_flags);
+ Assert(slot > 0 && slot <= num_child_inuse);
slot--;
/*
@@ -264,17 +295,18 @@ ReleasePostmasterChildSlot(int slot)
*/
result = (PMSignalState->PMChildFlags[slot] == PM_CHILD_ASSIGNED);
PMSignalState->PMChildFlags[slot] = PM_CHILD_UNUSED;
+ PMChildInUse[slot] = false;
return result;
}
/*
* IsPostmasterChildWalSender - check if given slot is in use by a
- * walsender process.
+ * walsender process. This is called only by the postmaster.
*/
bool
IsPostmasterChildWalSender(int slot)
{
- Assert(slot > 0 && slot <= PMSignalState->num_child_flags);
+ Assert(slot > 0 && slot <= num_child_inuse);
slot--;
if (PMSignalState->PMChildFlags[slot] == PM_CHILD_WALSENDER)
Re: use has_privs_of_role() for pg_hba.conf
On Sat, Oct 08, 2022 at 09:57:02AM -0700, David G. Johnston wrote: > I would tend to agree that even membership probably shouldn't be involved > here, and that this entire feature would be implemented in an orthogonal > manner. I don't see any specific need to try and move to a more isolated > implementation, but trying to involve inheritance just seems wrong. The > status quo seems like a good place to stay. Okay, I think there are sufficient votes against this change to simply mark it Rejected. Thanks for the discussion! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: use has_privs_of_role() for pg_hba.conf
On Sat, Oct 08, 2022 at 11:46:50AM -0400, Robert Haas wrote: > Now there may be some other scenario in which the patch is going in > exactly the right direction, and if I knew what it was, maybe I'd > agree that the patch was a great idea. But I haven't seen anything > like that on the thread. Basically, the argument is just that the > change would make things more consistent. However, it might be an > abuse of the term. If you go out and buy blue curtains because you > have a blue couch, that's consistent interior decor. If you go out and > buy a blue car because you have a blue couch, that's not really > consistent anything, it's just two fairly-unrelated things that are > both blue. I believe I started this thread after reviewing the remaining uses of is_member_of_role() after 6198420 was committed and wondering whether this case was an oversight. If upon closer inspection we think that mere membership is appropriate for pg_hba.conf, I'm fully prepared to go and mark this commitfest entry as Rejected. It obviously does not seem as clear-cut as 6198420. And I'll admit I don't have a concrete use-case in hand to justify the behavior change. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: use has_privs_of_role() for pg_hba.conf
On Sat, Oct 8, 2022 at 8:47 AM Robert Haas wrote: > On Sat, Oct 8, 2022 at 11:14 AM Tom Lane wrote: > > Joe Conway writes: > > > Thanks -- looks good to me. If there are no other comments or concerns, > > > I will commit/push by the end of the weekend. > > > > Robert seems to think that this patch might be completely misguided, > > so I'm not sure we have real consensus. I think he may have a point. > > I think what is bothering me is a feeling that a privilege is > something that you get because you've authenticated. If you haven't > authenticated yet, you have no privileges. So why should it matter > whether the role to which you could hypothetically authenticate would > inherit the privileges of some other role or not? > > Or to put it another way, I don't have any intuition for why someone > would want the system to behave in this way rather than in the way > that it does now. > I'm also in the "inheritance isn't relevant here" camp. One doesn't inherit an ability to LOGIN from a group that has a LOGIN attribute. The [NO]INHERIT attribute doesn't even apply. This feature is so closely related to LOGIN that [NO]INHERIT should likewise not apply here as well. We've decided to conjoin two arguably orthogonal concerns here and need to keep in mind that any given aspect of the overall capability might very well only apply to a subset of the system. In this case inheritance only applies to object permissions, not attributes, and not authentication (which doesn't have any kind of explicit permission bit in the system to inherit, making it just like LOGIN). I would tend to agree that even membership probably shouldn't be involved here, and that this entire feature would be implemented in an orthogonal manner. I don't see any specific need to try and move to a more isolated implementation, but trying to involve inheritance just seems wrong. The status quo seems like a good place to stay. David J.
Re: START_REPLICATION SLOT causing a crash in an assert build
Hi, On 2022-10-07 19:56:33 -0700, Andres Freund wrote: > I'm planning to push this either later tonight (if I feel up to it after > cooking dinner) or tomorrow morning PST, due to the release wrap deadline. I looked this over again, tested a bit more, and pushed the adjusted 15 and master versions to github to get a CI run. Once that passes, as I expect, I'll push them for real. Greetings, Andres Freund
Re: Reducing the chunk header sizes on all memory context types
So I pushed that, but I don't feel that we're out of the woods yet.
As I mentioned at [1], while testing this stuff I hit a case where
aset.c will try to wipe_mem practically the entire address space after
being asked to pfree() an invalid pointer. The specific reproducer
isn't too interesting because it depends on the pre-80ef92675 state of
the code, but what I take away from this is that aset.c is still far
too fragile as it stands.
One problem is that aset.c generally isn't too careful about whether
a putative chunk is actually one of its chunks. That was okay before
c6e0fe1f2 because we would never get to AllocSetFree etc unless the
word before the chunk data pointed at a moderately-sane AllocSet.
Now, we can arrive at that code on the strength of three random bits,
so it's got to be more careful. In the attached patch, I make
AllocSetIsValid do an IsA() test, and make sure to apply it to the
aset we think we have found from the chunk header. This is not in
any way a new check: what it is doing is replacing the IsA check done
by the "AssertArg(MemoryContextIsValid(context))" that was performed
by GetMemoryChunkContext in the old code, and that we've completely
lost in the code as it stands.
The other problem, which is what is leading to wipe_mem-the-universe,
is that aset.c figures the size of a non-external chunk essentially
as "1 << MemoryChunkGetValue(chunk)", where the "value" field is 30
bits wide and has undergone exactly zero validation before
AllocSetFree uses the size in memset. That's far, far too trusting.
In the attached I put in some asserts to verify that the value field
is in the valid range for a freelist index, which should usually
trigger if we have a garbage value, or at the very least constrain
the damage.
What I am mainly wondering about at this point is whether Asserts
are good enough or we should use actual test-and-elog checks for
these things. The precedent of the old GetMemoryChunkContext
implementation suggests that assertions are good enough for the
AllocSetIsValid tests. On the other hand, there are existing
cross-checks like
if (block->freeptr != block->endptr)
elog(ERROR, "could not find block containing chunk %p", chunk);
so at least in some code paths we've thought it is worth expending
such tests in production builds. Any opinions?
I imagine generation.c and slab.c need similar bulletproofing
measures, but I didn't look at them yet.
regards, tom lane
[1] https://www.postgresql.org/message-id/3436789.1665187055%40sss.pgh.pa.us
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec423375ae..76a9f02bd8 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -132,6 +132,10 @@ typedef struct AllocFreeListLink
#define GetFreeListLink(chkptr) \
(AllocFreeListLink *) ((char *) (chkptr) + ALLOC_CHUNKHDRSZ)
+/* Validate a freelist index retrieved from a chunk header */
+#define FreeListIdxIsValid(fidx) \
+ ((fidx) >= 0 && (fidx) < ALLOCSET_NUM_FREELISTS)
+
/* Determine the size of the chunk based on the freelist index */
#define GetChunkSizeFromFreeListIdx(fidx) \
Size) 1) << ALLOC_MINBITS) << (fidx))
@@ -202,7 +206,15 @@ typedef struct AllocBlockData
* AllocSetIsValid
* True iff set is valid allocation set.
*/
-#define AllocSetIsValid(set) PointerIsValid(set)
+#define AllocSetIsValid(set) \
+ (PointerIsValid(set) && IsA(set, AllocSetContext))
+
+/*
+ * AllocBlockIsValid
+ * True iff block is valid block of allocation set.
+ */
+#define AllocBlockIsValid(block) \
+ (PointerIsValid(block) && AllocSetIsValid((block)->aset))
/*
* We always store external chunks on a dedicated block. This makes fetching
@@ -530,8 +542,7 @@ AllocSetReset(MemoryContext context)
{
AllocSet set = (AllocSet) context;
AllocBlock block;
- Size keepersize PG_USED_FOR_ASSERTS_ONLY
- = set->keeper->endptr - ((char *) set);
+ Size keepersize PG_USED_FOR_ASSERTS_ONLY;
AssertArg(AllocSetIsValid(set));
@@ -540,6 +551,9 @@ AllocSetReset(MemoryContext context)
AllocSetCheck(context);
#endif
+ /* Remember keeper block size for Assert below */
+ keepersize = set->keeper->endptr - ((char *) set);
+
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -598,8 +612,7 @@ AllocSetDelete(MemoryContext context)
{
AllocSet set = (AllocSet) context;
AllocBlock block = set->blocks;
- Size keepersize PG_USED_FOR_ASSERTS_ONLY
- = set->keeper->endptr - ((char *) set);
+ Size keepersize PG_USED_FOR_ASSERTS_ONLY;
AssertArg(AllocSetIsValid(set));
@@ -608,6 +621,9 @@ AllocSetDelete(MemoryContext context)
AllocSetCheck(context);
#endif
+ /* Remember keeper block size for Assert below */
+ keepersize = set->keeper->endptr - ((char *) set);
+
/*
* If the context is a candidate for a freelist, put it into that freelist
* instead of destroying it.
@@ -994,9 +1010,10 @@ AllocSetFree(void *pointer)
if
Re: use has_privs_of_role() for pg_hba.conf
On Sat, Oct 8, 2022 at 11:14 AM Tom Lane wrote: > Joe Conway writes: > > Thanks -- looks good to me. If there are no other comments or concerns, > > I will commit/push by the end of the weekend. > > Robert seems to think that this patch might be completely misguided, > so I'm not sure we have real consensus. I think he may have a point. > > An angle that he didn't bring up is that we've had proposals, and > even I think a patch, for inventing database-local privileges. > If that were to become a thing, it would interact very badly with > this idea, because it would often not be clear which set of privileges > to consider. As long as HBA checks consider membership, and we don't > invent database-local role membership, there's no problem. This argument feels a little bit thin to me, because (1) one could equally well postulate that we'd want to invent database-local role membership and (2) presumably the relevant set of privileges would be those for the database to which the user wishes to authenticate. I think what is bothering me is a feeling that a privilege is something that you get because you've authenticated. If you haven't authenticated yet, you have no privileges. So why should it matter whether the role to which you could hypothetically authenticate would inherit the privileges of some other role or not? Or to put it another way, I don't have any intuition for why someone would want the system to behave in this way rather than in the way that it does now. In general, what role inheritance does is actually pretty easy to understand: either you just have the ability to access the privileges of some other role at need, or you have those privileges all the time even without activating them explicitly. I think in most cases people will expect membership in a predefined role or a role used as a group to behave in the second way, and membership in a login role to be used in the first way, but I think there will likely be some exceptions in both directions, which is fine, because we can support that. But the usage where you mention a group in pg_hba.conf feels orthogonal to all of that to me. In that case, it's not really about privileges at all, or at least I don't think so. It's about letting one group of people log into the system from, say, a certain IP address, and others not (or maybe the reverse). It seems reasonably likely that you wouldn't want the role you used for grouping purposes in a case like this to hold any privileges at all, or that if it did have any privileges you wouldn't want them accessible in any way to the group members, because if you create a group called people_who_can_log_in_from_the_modem_pool, you do not therefore want to end up with tables owned by people_who_can_log_in_from_the_modem_pool. Under that theory, this patch is going in the wrong direction. Now there may be some other scenario in which the patch is going in exactly the right direction, and if I knew what it was, maybe I'd agree that the patch was a great idea. But I haven't seen anything like that on the thread. Basically, the argument is just that the change would make things more consistent. However, it might be an abuse of the term. If you go out and buy blue curtains because you have a blue couch, that's consistent interior decor. If you go out and buy a blue car because you have a blue couch, that's not really consistent anything, it's just two fairly-unrelated things that are both blue. -- Robert Haas EDB: http://www.enterprisedb.com
Re: use has_privs_of_role() for pg_hba.conf
Joe Conway writes: > Thanks -- looks good to me. If there are no other comments or concerns, > I will commit/push by the end of the weekend. Robert seems to think that this patch might be completely misguided, so I'm not sure we have real consensus. I think he may have a point. An angle that he didn't bring up is that we've had proposals, and even I think a patch, for inventing database-local privileges. If that were to become a thing, it would interact very badly with this idea, because it would often not be clear which set of privileges to consider. As long as HBA checks consider membership, and we don't invent database-local role membership, there's no problem. regards, tom lane
Re: use has_privs_of_role() for pg_hba.conf
On 10/7/22 17:58, Nathan Bossart wrote: On Fri, Oct 07, 2022 at 04:18:59PM -0400, Tom Lane wrote: There's another problem there, which is that buildfarm animals using -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS will complain about role names that don't start with "regress_". Huh, I hadn't noticed that one before. It looks like roles must start with "regress_" and database names must include "regression", so I ended up using "regress_regression_group" for the samegroup/samerole tests. Thanks -- looks good to me. If there are no other comments or concerns, I will commit/push by the end of the weekend. -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
