Re: Using PQexecQuery in pipeline mode produces unexpected Close messages
At Tue, 21 Jun 2022 14:56:40 +0900 (JST), Kyotaro Horiguchi wrote in > By the way, I noticed that "libpq_pipeline uniqviol" intermittently > fails for uncertain reasons. > > > result 574/575: pipeline aborted > > ... > > done writing > > > > libpq_pipeline:1531: got unexpected NULL > > The "...done writing" is printed too late in the error cases. > > This causes the TAP test fail, but I haven't find what's happnening at > the time. Just to make sure, I see this with the master branch regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Using PQexecQuery in pipeline mode produces unexpected Close messages
At Tue, 21 Jun 2022 11:42:59 +0900 (JST), Kyotaro Horiguchi
wrote in
> At Fri, 17 Jun 2022 20:31:50 +0200, Alvaro Herrera
> wrote in
> > Others to think about:
> >
> > PQisBusy (I think no changes are needed),
>
> Agreed.
>
> > PQfn (I think it should accept a call in PGASYNC_PIPELINE_IDLE mode;
> > fully untested in pipeline mode),
>
> Does a PQ_PIPELINE_OFF path need that? Rather I thought that we need
> Assert(!conn->asyncStatus != PGASYNC_PIPELINE_IDLE) there. But anyway
> we might need a test for this path.
In the attached, PQfn() is used while in pipeline mode and
PGASYNC_PIPELINE_IDLE. Both error out and effectivelly no-op.
> > PQexitPipelineMode (I think it needs to return error; needs test case),
>
> Agreed about test case. Currently the function doesn't handle
> PGASYNC_IDLE so I thought that PGASYNC_PIPELINE_IDLE also don't need a
> care. If the function treats PGASYNC_PIPELINE_IDLE state as error,
> the regression test fails (but I haven't examine it furtuer.)
PQexitPipelineMode() is called while PGASYNC_PIPELINE_IDLE.
> > PQsendFlushRequest (I think it should let through; ditto).
>
> Does that mean exit without pushing 'H' message?
I didn't do anything on this in the sttached.
By the way, I noticed that "libpq_pipeline uniqviol" intermittently
fails for uncertain reasons.
> result 574/575: pipeline aborted
> ...
> done writing
>
> libpq_pipeline:1531: got unexpected NULL
The "...done writing" is printed too late in the error cases.
This causes the TAP test fail, but I haven't find what's happnening at
the time.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 0ff563f59a..84734f5d41 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -53,6 +53,8 @@ static const char *const insert_sql =
static const char *const insert_sql2 =
"INSERT INTO pq_pipeline_demo(itemno,int8filler) VALUES ($1, $2)";
+Oid oid_pg_backend_pid;
+
/* max char length of an int32/64, plus sign and null terminator */
#define MAXINTLEN 12
#define MAXINT8LEN 20
@@ -86,6 +88,24 @@ pg_fatal_impl(int line, const char *fmt,...)
exit(1);
}
+/* returns true if PQfn succeeded */
+static bool
+PQfn_succeed(PGconn *conn)
+{
+ PGresult *res;
+ int res_buf;
+ int res_len;
+
+ /* PQfn should fail in pipeline mode */
+ if ((res = PQfn(conn, oid_pg_backend_pid, _buf, _len, 1, NULL, 0))
+ != NULL &&
+ PQresultStatus(res) == PGRES_COMMAND_OK)
+ return true;
+
+ return false;
+}
+
+
static void
test_disallowed_in_pipeline(PGconn *conn)
{
@@ -93,6 +113,9 @@ test_disallowed_in_pipeline(PGconn *conn)
fprintf(stderr, "test error cases... ");
+ if (oid_pg_backend_pid == 0)
+ pg_fatal("Failed to obtain functino oid: %s", PQgetvalue(res, 0, 0));
+
if (PQisnonblocking(conn))
pg_fatal("Expected blocking connection mode");
@@ -107,6 +130,10 @@ test_disallowed_in_pipeline(PGconn *conn)
if (PQresultStatus(res) != PGRES_FATAL_ERROR)
pg_fatal("PQexec should fail in pipeline mode but succeeded");
+ /* PQfn should fail in pipeline mode */
+ if (PQfn_succeed(conn))
+ pg_fatal("PQfn succeeded while in pipeline mode");
+
/* Entering pipeline mode when already in pipeline mode is OK */
if (PQenterPipelineMode(conn) != 1)
pg_fatal("re-entering pipeline mode should be a no-op but failed");
@@ -1014,6 +1041,18 @@ test_simple_pipeline(PGconn *conn)
PQclear(res);
res = NULL;
+ /* PQfn shoud be error no-op while PGASYNC_PIPELINE_IDLE */
+ if (PQfn_succeed(conn))
+ pg_fatal("PQfn succeeded while in pipeline mode");
+
+ /*
+ * Trial to getting out of pipeline mode while PGASYNC_PIPELINE_IDLE should
+ * gracefully fail.
+ */
+ if (PQexitPipelineMode(conn) != 0)
+ pg_fatal("exiting pipeline mode after query but before sync succeeded incorrectly");
+
+ /* This PQgetResult moves async status to PGASYNC_BUSY */
if (PQgetResult(conn) != NULL)
pg_fatal("PQgetResult returned something extra after first query result.");
@@ -1627,6 +1666,15 @@ main(int argc, char **argv)
if (PQresultStatus(res) != PGRES_COMMAND_OK)
pg_fatal("failed to set force_parallel_mode: %s", PQerrorMessage(conn));
+ /* obtain function OID of pg_backend_pid for PQfn test */
+ res = PQexec(conn, "SELECT 'pg_backend_pid'::regproc::int");
+ if (!res ||
+ (PQresultStatus(res) != PGRES_COMMAND_OK &&
+ PQresultStatus(res) != PGRES_TUPLES_OK))
+ pg_fatal("Failed to obtain functino oid");
+
+ oid_pg_backend_pid = atoi(PQgetvalue(res, 0, 0));
+
/* Set the trace file, if requested */
if (tracefile != NULL)
{
Re: Replica Identity check of partition table on subscriber
On Tue, Jun 21, 2022 at 8:02 AM Amit Kapila wrote: > > On Tue, Jun 21, 2022 at 7:49 AM Amit Langote wrote: > > > > > > I think it should spell out REPLICA IDENTITY explicitly to avoid the > > commit being confused to have to do with "Referential Integrity > > checking". > > > > This makes sense. I'll take care of this. > After pushing this patch, buildfarm member prion has failed. https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=prion=HEAD It seems to me that the problem could be due to the reason that the entry returned by logicalrep_partition_open() may not have the correct value for localrel when we found the entry and localrelvalid is also true. The point is that before this commit we never use localrel value from the rel entry returned by logicalrep_partition_open. I think we need to always update the localrel value in logicalrep_partition_open(). -- With Regards, Amit Kapila.
Re: Is RecoveryConflictInterrupt() entirely safe in a signal handler?
Here's a new version, but there's something wrong that I haven't figured out yet (see CI link below). Here's one thing I got a bit confused about along the way, but it seems the comment was just wrong: + /* +* If we can abort just the current subtransaction then we are OK +* to throw an ERROR to resolve the conflict. Otherwise drop +* through to the FATAL case. ... + if (!IsSubTransaction()) ... + ereport(ERROR, Surely this was meant to say, "If we're not in a subtransaction then ...", right? Changed. I thought of a couple more simplifications for the big switch statement in ProcessRecoveryConflictInterrupt(). The special case for DoingCommandRead can be changed to fall through, instead of needing a separate ereport(FATAL). I also folded the two ereport(FATAL) calls in the CONFLICT_DATABASE case into one, since they differ only in errcode(). + (errcode(reason == PROCSIG_RECOVERY_CONFLICT_DATABASE ? + ERRCODE_DATABASE_DROPPED : + ERRCODE_T_R_SERIALIZATION_FAILURE), Now we're down to just one ereport(FATAL), one ereport(ERROR), and a couple of ways to give up without erroring. I think this makes the logic a lot easier to follow? I'm confused about proc->recoveryConflictPending: the startup process sets it, and sometimes the interrupt receiver sets it too, and it causes errdetail() to be clobbered on abort (for any reason), even though we bothered to set it carefully for the recovery conflict ereport calls. Or something like that. I haven't changed anything about that in this patch, though. Problem: I saw 031_recovery_conflict.pl time out while waiting for a buffer pin conflict, but so far once only, on CI: https://cirrus-ci.com/task/5956804860444672 timed out waiting for match: (?^:User was holding shared buffer pin for too long) at t/031_recovery_conflict.pl line 367. Hrmph. Still trying to reproduce that, which may be a bug in this patch, a bug in the test or a pre-existing problem. Note that recovery didn't say something like: 2022-06-21 17:05:40.931 NZST [57674] LOG: recovery still waiting after 11.197 ms: recovery conflict on buffer pin (That's what I'd expect to see in https://api.cirrus-ci.com/v1/artifact/task/5956804860444672/log/src/test/recovery/tmp_check/log/031_recovery_conflict_standby.log if the startup process had decided to send the signal). ... so it seems like the problem in that run is upstream of the interrupt stuff. Other things changed in response to feedback (quoting from several recent messages): On Thu, Jun 16, 2022 at 5:01 AM Robert Haas wrote: > On Wed, Jun 15, 2022 at 1:51 AM Michael Paquier wrote: > > Handle is more consistent with the other types of interruptions in the > > SIGUSR1 handler, so the name proposed in the patch in not that > > confusing to me. And so does Process, in spirit with > > ProcessProcSignalBarrier() and ProcessLogMemoryContextInterrupt(). > > While on it, is postgres.c the best home for > > HandleRecoveryConflictInterrupt()? That's a very generic file, for > > starters. Not related to the actual bug, just asking. > > Yeah, there's existing precedent for this kind of split in, for > example, HandleCatchupInterrupt() and ProcessCatchupInterrupt(). I > think the idea is that "process" is supposed to sound like the more > involved part of the operation, whereas "handle" is supposed to sound > like the initial response to the signal. Thanks both for looking. Yeah, I was trying to keep with the existing convention here (though admittedly we're not 100% consistent on this, something to tidy up separately perhaps). On Wed, Jun 15, 2022 at 5:51 PM Michael Paquier wrote: > On Sun, May 22, 2022 at 05:10:01PM -0700, Andres Freund wrote: > > It *might* look a tad cleaner to have the loop in a separate function from > > the > > existing code. I.e. a +ProcessRecoveryConflictInterrupts() that calls > > ProcessRecoveryConflictInterrupts(). > > Agreed that it would be a bit cleaner to keep the internals in a > different routine. Alright, I split it into two functions: one with an 's' in the name to do the looping, and one without 's' to process an individual interrupt reason. Makes the patch harder to read because the indentation level changes... > Also note that bufmgr.c mentions RecoveryConflictInterrupt() in the > top comment of HoldingBufferPinThatDelaysRecovery(). Fixed. > Should the processing of PROCSIG_RECOVERY_CONFLICT_DATABASE mention > that FATAL is used because we are never going to retry the conflict as > the database has been dropped? OK, note added. > Getting rid of > RecoveryConflictRetryable makes the code easier to go through. Yeah, all the communication through global variables was really confusing, and also subtly wrong (that global reason gets clobbered with incorrect values), and that retryable variable was hard to follow. On
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jun 21, 2022 at 7:11 AM Peter Smith wrote: > > Here are some review comments for the v11-0001 patch. > > (I will review the remaining patches 0002-0005 and post any comments later) > > == > > 1. General > > I still feel that 'apply' seems like a meaningless enum value for this > feature because from a user point-of-view every replicated change gets > "applied". IMO something like 'streaming = parallel' or 'streaming = > background' (etc) might have more meaning for a user. > +1. I would prefer 'streaming = parallel' as that suits here because we allow streams (set of changes) of a transaction to be applied in parallel to other transactions or in parallel to a stream of changes from another streaming transaction. > == > > 10. src/backend/access/transam/xact.c > > @@ -1741,6 +1742,13 @@ RecordTransactionAbort(bool isSubXact) > elog(PANIC, "cannot abort transaction %u, it was already committed", > xid); > > + /* > + * Are we using the replication origins feature? Or, in other words, > + * are we replaying remote actions? > + */ > + replorigin = (replorigin_session_origin != InvalidRepOriginId && > + replorigin_session_origin != DoNotReplicateId); > + > /* Fetch the data we need for the abort record */ > nrels = smgrGetPendingDeletes(false, ); > nchildren = xactGetCommittedChildren(); > @@ -1765,6 +1773,11 @@ RecordTransactionAbort(bool isSubXact) > MyXactFlags, InvalidTransactionId, > NULL); > > + if (replorigin) > + /* Move LSNs forward for this replication origin */ > + replorigin_session_advance(replorigin_session_origin_lsn, > +XactLastRecEnd); > + > > I did not see any reason why the code assigning the 'replorigin' and > the code checking the 'replorigin' are separated like they are. I > thought these 2 new code fragments should be kept together. Perhaps it > was decided this assignment must be outside the critical section? But > if that’s the case maybe a comment explaining so would be good. > I also don't see any particular reason for this apart from being similar to RecordTransactionCommit(). I think it should be fine either way. > ~~~ > > 11. src/backend/access/transam/xact.c > > + if (replorigin) > + /* Move LSNs forward for this replication origin */ > + replorigin_session_advance(replorigin_session_origin_lsn, > + > > The positioning of that comment is unusual. Maybe better before the check? > This again seems to be due to a similar code in RecordTransactionCommit(). I would suggest let's keep the code consistent. -- With Regards, Amit Kapila.
Re: Using PQexecQuery in pipeline mode produces unexpected Close messages
At Fri, 17 Jun 2022 20:31:50 +0200, Alvaro Herrera wrote in > On 2022-Jun-16, Kyotaro Horiguchi wrote: > > > The attached is a crude patch to separate the state for PIPELINE-IDLE > > from PGASYNC_IDLE. I haven't found a better way.. > > Ah, yeah, this might be a way to fix this. > > Something very similar to a PIPELINE_IDLE mode was present in Craig's > initial patch for pipeline mode. However, I fought very hard to remove > it, because it seemed to me that failing to handle it correctly > everywhere would lead to more bugs than not having it. (Indeed, there > were some.) > > However, I see now that your patch would not only fix this bug, but also > let us remove the ugly "notionally not-idle" bit in fe-protocol3.c, > which makes me ecstatic. So let's push forward with this. However, Yey. > this means we'll have to go over all places that use asyncStatus to > ensure that they all handle the new value correctly. Sure. > I did found one bug in your patch: in the switch for asyncStatus in > PQsendQueryStart, you introduce a new error message. With the current > tests, that never fires, which is telling us that our coverage is not > complete. But with the right sequence of libpq calls, which the > attached adds (note that it's for REL_14_STABLE), that can be hit, and # (ah, I wondered why it failed to apply..) > it's easy to see that throwing an error there is a mistake. The right > action to take there is to let the action through. Yeah.. Actulallly I really did it carelessly.. Thanks! > Others to think about: > > PQisBusy (I think no changes are needed), Agreed. > PQfn (I think it should accept a call in PGASYNC_PIPELINE_IDLE mode; > fully untested in pipeline mode), Does a PQ_PIPELINE_OFF path need that? Rather I thought that we need Assert(!conn->asyncStatus != PGASYNC_PIPELINE_IDLE) there. But anyway we might need a test for this path. > PQexitPipelineMode (I think it needs to return error; needs test case), Agreed about test case. Currently the function doesn't handle PGASYNC_IDLE so I thought that PGASYNC_PIPELINE_IDLE also don't need a care. If the function treats PGASYNC_PIPELINE_IDLE state as error, the regression test fails (but I haven't examine it furtuer.) > PQsendFlushRequest (I think it should let through; ditto). Does that mean exit without pushing 'H' message? > I also attach a patch to make the test suite use Test::Differences, if > available. It makes the diffs of the traces much easier to read, when > they fail. (I wish for a simple way to set the context size, but that > would need a shim routine that I'm currently too lazy to write.) Yeah, it was annoying that the script prints expected and result trace separately. It looks pretty good with the patch. I don't think there's much use of context size here. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Replica Identity check of partition table on subscriber
On Tue, Jun 21, 2022 at 7:49 AM Amit Langote wrote: > > On Mon, Jun 20, 2022 at 3:46 PM shiy.f...@fujitsu.com > wrote: > > On Mon, Jun 20, 2022 1:33 PM Amit Kapila wrote: > > > One minor comment: > > > + /* > > > + * If it is a partitioned table, we don't check it, we will check its > > > + * partition later. > > > + */ > > > > > > Can we change the above comment to: "For partitioned tables, we only > > > need to care if the target partition is updatable (aka has PK or RI > > > defined for it)."? > > > > > Thanks for your comment. Modified in the attached patches. > > How about: ...target "leaf" partition is updatable > I am not very sure if this is an improvement over the current. > Regarding the commit message's top line, which is this: > > Fix partition table's RI checking on the subscriber. > > I think it should spell out REPLICA IDENTITY explicitly to avoid the > commit being confused to have to do with "Referential Integrity > checking". > This makes sense. I'll take care of this. -- With Regards, Amit Kapila.
Re: Replica Identity check of partition table on subscriber
On Mon, Jun 20, 2022 at 3:46 PM shiy.f...@fujitsu.com wrote: > On Mon, Jun 20, 2022 1:33 PM Amit Kapila wrote: > > One minor comment: > > + /* > > + * If it is a partitioned table, we don't check it, we will check its > > + * partition later. > > + */ > > > > Can we change the above comment to: "For partitioned tables, we only > > need to care if the target partition is updatable (aka has PK or RI > > defined for it)."? > > > Thanks for your comment. Modified in the attached patches. How about: ...target "leaf" partition is updatable Regarding the commit message's top line, which is this: Fix partition table's RI checking on the subscriber. I think it should spell out REPLICA IDENTITY explicitly to avoid the commit being confused to have to do with "Referential Integrity checking". -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths
On Mon, Jun 20, 2022 at 11:01:51AM +0200, Matthias van de Meent wrote: > On Mon, 20 Jun 2022 at 07:02, Michael Paquier wrote: >> + if (unlikely(!AllocSizeIsValid(total_len))) >> + XLogErrorDataLimitExceeded(); >> Rather than a single check at the end of XLogRecordAssemble(), you'd >> better look after that each time total_len is added up? > > I was doing so previously, but there were some good arguments against that: > > - Performance of XLogRecordAssemble should be impacted as little as > possible. XLogRecordAssemble is in many hot paths, and it is highly > unlikely this check will be hit, because nobody else has previously > reported this issue. Any check, however unlikely, will add some > overhead, so removing check counts reduces overhead of this patch. Some macro-benchmarking could be in place here, and this would most likely become noticeable when assembling a bunch of little records? > - The user or system is unlikely to care about which specific check > was hit, and only needs to care _that_ the check was hit. An attached > debugger will be able to debug the internals of the xlog machinery and > find out the specific reasons for the error, but I see no specific > reason why the specific reason would need to be reported to the > connection. Okay. + /* +* Ensure that xlogreader.c can read the record by ensuring that the +* data section of the WAL record can be allocated. +*/ + if (unlikely(!AllocSizeIsValid(total_len))) + XLogErrorDataLimitExceeded(); By the way, while skimming through the patch, the WAL reader seems to be a bit more pessimistic than this estimation, calculating the amount to allocate as of DecodeXLogRecordRequiredSpace(), based on the xl_tot_len given by a record. -- Michael signature.asc Description: PGP signature
Re: Perform streaming logical transactions by background workers and parallel apply
Here are some review comments for the v11-0001 patch. (I will review the remaining patches 0002-0005 and post any comments later) == 1. General I still feel that 'apply' seems like a meaningless enum value for this feature because from a user point-of-view every replicated change gets "applied". IMO something like 'streaming = parallel' or 'streaming = background' (etc) might have more meaning for a user. == 2. Commit message We also need to allow stream_stop to complete by the apply background worker to avoid deadlocks because T-1's current stream of changes can update rows in conflicting order with T-2's next stream of changes. Did this mean to say? "allow stream_stop to complete by" -> "allow stream_stop to be performed by" ~~~ 3. Commit message This patch also extends the SUBSCRIPTION 'streaming' option so that the user can control whether to apply the streaming transaction in an apply background worker or spill the change to disk. User can set the streaming option to 'on/off', 'apply'. For now, 'apply' means the streaming will be applied via a apply background worker if available. 'on' means the streaming transaction will be spilled to disk. 3a. "option" -> "parameter" (2x) 3b. "User can" -> "The user can" 3c. I think this part should also mention that the stream parameter default is unchanged... == 4. doc/src/sgml/config.sgml + +Maximum number of apply background workers per subscription. This +parameter controls the amount of parallelism of the streaming of +in-progress transactions if we set subscription option +streaming to apply. + "if we set subscription option streaming to apply." -> "when subscription parameter streaming = apply. == 5. doc/src/sgml/config.sgml + + Setting streaming mode to apply could export invalid LSN + as finish LSN of failed transaction. Changing the streaming mode and making + the same conflict writes the finish LSN of the failed transaction in the + server log if required. + This text made no sense to me. Can you reword it? IIUC it means something like this: When the streaming mode is 'apply', the finish LSN of failed transactions may not be logged. In that case, it may be necessary to change the streaming mode and cause the same conflicts again so the finish LSN of the failed transaction will be written to the server log. == 6. doc/src/sgml/protocol.sgml Since there are protocol changes made here, shouldn’t there also be some corresponding LOGICALREP_PROTO_XXX constants and special checking added in the worker.c? == 7. doc/src/sgml/ref/create_subscription.sgml + for this subscription. The default value is off, + all transactions are fully decoded on the publisher and only then + sent to the subscriber as a whole. + SUGGESTION The default value is off, meaning all transactions are fully decoded on the publisher and only then sent to the subscriber as a whole. ~~~ 8. doc/src/sgml/ref/create_subscription.sgml + + If set to on, the changes of transaction are + written to temporary files and then applied at once after the + transaction is committed on the publisher. + SUGGESTION If set to on, the incoming changes are written to a temporary file and then applied only after the transaction is committed on the publisher. ~~~ 9. doc/src/sgml/ref/create_subscription.sgml + + If set to apply incoming + changes are directly applied via one of the background workers, if + available. If no background worker is free to handle streaming + transaction then the changes are written to a file and applied after + the transaction is committed. Note that if an error happens when + applying changes in a background worker, it might not report the + finish LSN of the remote transaction in the server log. SUGGESTION If set to apply, the incoming changes are directly applied via one of the apply background workers, if available. If no background worker is free to handle streaming transactions then the changes are written to a file and applied after the transaction is committed. Note that if an error happens when applying changes in a background worker, the finish LSN of the remote transaction might not be reported in the server log. == 10. src/backend/access/transam/xact.c @@ -1741,6 +1742,13 @@ RecordTransactionAbort(bool isSubXact) elog(PANIC, "cannot abort transaction %u, it was already committed", xid); + /* + * Are we using the replication origins feature? Or, in other words, + * are we replaying remote actions? + */ + replorigin = (replorigin_session_origin != InvalidRepOriginId && + replorigin_session_origin != DoNotReplicateId); + /* Fetch the data we need for the abort record */ nrels = smgrGetPendingDeletes(false, ); nchildren = xactGetCommittedChildren(); @@ -1765,6 +1773,11 @@
Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
At Mon, 20 Jun 2022 11:57:20 -0400, Robert Haas wrote
in
> It seems to me that what we want to do is: if we're about to start
> allowing WAL writes, then consider whether to insert an aborted
> contrecord record. Now, if we are about to start allowing WAL write,
> we must determine the LSN at which the first such record will be
> written. Then there are two possibilities: either that LSN is on an
> existing record boundary, or it isn't. In the former case, no aborted
> contrecord record is required. In the latter case, we're writing at
> LSN which would have been in the middle of a previous record, except
> that the record in question was never finished or at least we don't
> have all of it. So the first WAL we insert at our chosen starting LSN
> must be an aborted contrecord record.
Right.
> I'm not quite sure I understand the nature of the remaining bug here,
> but this logic seems quite sketchy to me:
>
> /*
> * When not in standby mode we find that WAL ends in an incomplete
> * record, keep track of that record. After recovery is done,
> * we'll write a record to indicate to downstream WAL readers that
> * that portion is to be ignored.
> */
> if (!StandbyMode &&
> !XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
> {
> abortedRecPtr = xlogreader->abortedRecPtr;
> missingContrecPtr = xlogreader->missingContrecPtr;
> }
>
> I don't see what StandbyMode has to do with anything here. The
> question is whether the place where we're going to begin writing WAL
> is on an existing record boundary or not. If it so happens that when
> StandbyMode is turned on we can never decide to promote in the middle
> of a record, then maybe there's no need to track this information when
> StandbyMode = true, but I don't see a good reason why we should make
> that assumption. What if in the future somebody added a command that
Right. I came to the same conclusion before reading this section. It
is rearer than other cases but surely possible.
> says "don't keep trying to read more WAL, just promote RIGHT HERE?". I
> think this logic would surely be incorrect in that case. It feels to
> me like the right thing to do is to always keep track if WAL ends in
> an incomplete record, and then when we promote, we write an aborted
> contrecord record if WAL ended in an incomplete record, full stop.
Agreed. Actually, with the second patch applied, removing !StandbyMode
from the condition makes no difference in behavior.
> Now, we do need to keep in mind that, while in StandbyMode, we might
> reach the end of WAL more than once, because we have a polling loop
> that looks for more WAL. So it does not work to just set the values
> once and then assume that's the whole truth forever. But why not
> handle that by storing the abortedRecPtr and missingContrecPtr
> unconditionally after every call to XLogPrefetcherReadRecord()? They
> will go back and forth between XLogRecPtrIsInvalid and other values
> many times but the last value should -- I think -- be however things
> ended up at the point where we decided to stop reading WAL.
Unfortunately it doesn't work because we read a record already known
to be complete again at the end of recovery. It is the reason of
"abortedRecPtr < xlogreader->EndRecPtr" in my PoC patch. Without it,
abrotedRecPtr is erased when it is actually needed. I don't like that
expedient-looking condition, but the strict condition for resetting
abortedRecPtr is iff "we have read a complete record at the same or
grater LSN ofabortedRecPtr"...
Come to think of this, I noticed that we can get rid of the
file-global abortedRecPtr by letting FinishWalRecovery copy
abortedRecPtr *before* reading the last record. This also allows us to
remove the "expedient" condition.
> I haven't really dived into this issue much so I might be totally off
> base, but it just doesn't feel right to me that this should depend on
> whether we're in standby mode. That seems at best incidentally related
> to whether an aborted contrecord record is required.
>
> P.S. "aborted contrecord record" is really quite an awkward turn of phrase!
Thats true! Always open for better phrasings:(
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 6eba626420..c1be15039d 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -360,15 +360,6 @@ typedef struct XLogRecoveryCtlData
static XLogRecoveryCtlData *XLogRecoveryCtl = NULL;
-/*
- * abortedRecPtr is the start pointer of a broken record at end of WAL when
- * recovery completes; missingContrecPtr is the location of the first
- * contrecord that went missing. See CreateOverwriteContrecordRecord for
- * details.
- */
-static XLogRecPtr abortedRecPtr;
-static
Re: [PATCH] Completed unaccent dictionary with many missing characters
On Tue, Jun 21, 2022 at 12:11 PM Michael Paquier wrote: > Yeah, Latin-ASCII.xml is getting it wrong here, then. unaccent > fetches the thing from this URL currently: > https://raw.githubusercontent.com/unicode-org/cldr/release-41/common/transforms/Latin-ASCII.xml Oh, we're using CLDR 41, which reminds me: CLDR 36 added SOUND RECORDING COPYRIGHT[1] so we could drop it from special_cases(). Hmm, is it possible to get rid of CYRILLIC CAPITAL LETTER IO and CYRILLIC SMALL LETTER IO by adding Cyrillic to PLAIN_LETTER_RANGES? That'd leave just DEGREE CELSIUS and DEGREE FAHRENHEIT. Not sure how to kill those last two special cases -- they should be directly replaced by their decomposition. [1] https://unicode-org.atlassian.net/browse/CLDR-11383
Re: [PATCH] Completed unaccent dictionary with many missing characters
On Mon, Jun 20, 2022 at 10:37:57AM +0200, Przemysław Sztoch wrote: > But ligature check is performed on combining_ids (result of translation), > not on base codepoint. > Without it, you will get assertions in get_plain_letters. Hmm. I am wondering if we could make the whole logic a bit more intuitive here. The loop that builds the set of mappings gets now much more complicated with the addition of the categories beginning by N for the numbers, and that's mostly the same set of checks as the ones applied for T. > However, the current Latin-ASCII.xml suggests a conversion to x. > I found an open discussion on the internet about this and the suggestion > that the Latin-ASCII.xml file should be corrected for this letter. > But I wouldn't expect that Unicode makes the revised Latin-ASCII.xml quickly > into the official repo. Yeah, Latin-ASCII.xml is getting it wrong here, then. unaccent fetches the thing from this URL currently: https://raw.githubusercontent.com/unicode-org/cldr/release-41/common/transforms/Latin-ASCII.xml Could it be better to handle that as an exception in generate_unaccent_rules.py, documenting why we are doing it this way then? My concern is somebody re-running the script without noticing this exception, and the set of rules would be blindly, and incorrectly, updated. -- Michael signature.asc Description: PGP signature
Re: CFM for 2022-07
On Mon, Jun 20, 2022 at 12:57:22PM -0500, Jacob Champion wrote: > On Mon, Jun 20, 2022 at 12:38 PM Daniel Gustafsson wrote: >> Turning it on it's head, I'm happy to assist you. > > Thanks Daniel, that'd be great! Wow. Thanks, both of you. -- Michael signature.asc Description: PGP signature
Re: Remove trailing newlines from pg_upgrade's messages
Greg Stark writes: > On Wed, 15 Jun 2022 at 11:54, Tom Lane wrote: >> Yeah, that is sort of the inverse problem. I think those are there >> to ensure that the text appears on a fresh line even if the current >> line has transient status on it. We could get rid of those perhaps >> if we teach pg_log_v to remember whether it ended the last output >> with a newline or not, and then put out a leading newline only if >> necessary, rather than hard-wiring one into the message texts. > Is the problem that pg_upgrade doesn't know what the utilities it's > calling are outputting to the same terminal? Hmmm ... that's a point I'd not considered, but I think it's not an issue here. The subprograms generally have their output redirected to their own log files. regards, tom lane
Re: Remove trailing newlines from pg_upgrade's messages
On Wed, 15 Jun 2022 at 11:54, Tom Lane wrote: > > Yeah, that is sort of the inverse problem. I think those are there > to ensure that the text appears on a fresh line even if the current > line has transient status on it. We could get rid of those perhaps > if we teach pg_log_v to remember whether it ended the last output > with a newline or not, and then put out a leading newline only if > necessary, rather than hard-wiring one into the message texts. Is the problem that pg_upgrade doesn't know what the utilities it's calling are outputting to the same terminal? Another thing I wonder is if during development and testing there might have been more output from utilities or even the backend going on that are not happening now. -- greg
Re: Assorted small doc patches
On Wed, Jun 1, 2022 at 7:05 AM Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > On 31.05.22 22:12, David G. Johnston wrote: > > Anything I should be doing differently here to get a bit of > > reviewer/committer time on these? I'll add them to the commitfest for > > next month if needed but I'm seeing quick patches going in every week > > and the batch format done at the beginning of the month got processed > > through without issue. > > It might have been better to post these > separately. > > Doing so hasn't seemed to make a difference. Still not getting even a single committer on a single patch willing to either work with me to get it committed, or decide otherwise, despite my prompt responses on the few comments that have been given. Given the lack of feedback and my track record I don't think it unreasonable for someone to simply commit these without comment and let any late naysayers voice their opinions after it hits the repo. In any case, I've added these and more to the commitfest at this point so hopefully the changed mindset of committers when it comes to clearing out the commitfest backlog will come into play in a couple of weeks and I might see some action. David J.
Re: CFM for 2022-07
On Mon, Jun 20, 2022 at 12:38 PM Daniel Gustafsson wrote: > > On 20 Jun 2022, at 19:29, Jacob Champion wrote: > > (And if there is, I'm happy to assist and start learning the ropes, if that > > would be helpful.) > > Turning it on it's head, I'm happy to assist you. Thanks Daniel, that'd be great! --Jacob
Re: CFM for 2022-07
> On 20 Jun 2022, at 19:29, Jacob Champion wrote: > If there's not already a manager for the upcoming (July 2022) > commitfest, I'd like to volunteer. +1 > (And if there is, I'm happy to assist and start learning the ropes, if that > would be helpful.) Turning it on it's head, I'm happy to assist you. -- Daniel Gustafsson https://vmware.com/
CFM for 2022-07
Hi all, If there's not already a manager for the upcoming (July 2022) commitfest, I'd like to volunteer. (And if there is, I'm happy to assist and start learning the ropes, if that would be helpful.) --Jacob
Re: Finer grain log timestamps
On Mon, 20 Jun 2022 at 11:01, Tom Lane wrote: > Alvaro Herrera writes: > > If I were coding it, I would allow only exactly 1 digit (%.Nt) to simplify > the parsing side of things and bound the required buffer size. Without > having written it, it's not clear to me whether further restricting the > set of supported values would save much code. I will point out, though, > that throwing an error during log_line_prefix processing will lead > straight to infinite recursion. > I would parse the log_line_prefix when it is set. Then if there is a problem you just log it using whatever format is in effect and don't change the setting. Then the worst that happens is that logs show up in a format log processing isn't prepared to accept. That being said, I think I fall in the “just start putting more digits in the log” camp, although it is conceivable the counter arguments might be convincing.
Re: SLRUs in the main buffer pool, redux
On Thu, Jun 16, 2022 at 1:13 PM Konstantin Knizhnik wrote: > I wonder which workload can cause CLOG to become a bottleneck? > Usually Postgres uses hint bits to avoid clog access. So standard pgbench > doesn't demonstrate any degrade of performance even in case of presence of > long living transactions, > which keeps XMIN horizon. I haven't done research on this in a number of years and a bunch of other improvements have been made since then, but I remember discovering that CLOG could become a bottleneck on standard pgbench tests especially when using unlogged tables. The higher you raised the scale factor, the more of a bottleneck CLOG became. That makes sense: no matter what the scale factor is, you're constantly updating rows that have not previously been hinted, but as the scale factor gets bigger, those rows are likely to be older (in terms of XID age) on average, and so you need to cache more CLOG buffers to maintain performance. But the SLRU system provides no such flexibility: it doesn't scale to large numbers of buffers the way the code is written, and it certainly can't vary the number of buffers devoted to this purpose at runtime. So I think that the approach we're talking about here has potential in that sense. However, another problem with the SLRU code is that it's old and crufty and hard to work with. It's hard to imagine anyone being able to improve things very much as long as that's the basis. I don't know that whatever code Thomas has written or will write is better, but if it is, that would be good, because I don't see a lot of improvement in this area being possible otherwise. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
On Mon, Jun 20, 2022 at 7:28 AM Kyotaro Horiguchi
wrote:
> > Hmm. I have not looked at that in depth, but if the intention is to
> > check that the database is able to write WAL, looking at
> > XLogCtl->SharedRecoveryState would be the way to go because that's the
> > flip switching between crash recovery, archive recovery and the end of
> > recovery (when WAL can be safely written).
>
> What we are checking there is "we are going to write WAL", not "we are
> writing".
>
> (!StanbyMode && StandbyModeRequested) means the server have been
> running crash-recovery before starting archive recovery. In that
> case, the server is supposed to continue with archived WAL without
> insering a record. However, if no archived WAL available and the
> server promoted, the server needs to insert an "aborted contrecord"
> record again. (I'm not sure how that case happens in the field,
> though.)
>
> So I don't think !StandbyModeRequested is the right thing
> here. Actually the attached test fails with the fix.
It seems to me that what we want to do is: if we're about to start
allowing WAL writes, then consider whether to insert an aborted
contrecord record. Now, if we are about to start allowing WAL write,
we must determine the LSN at which the first such record will be
written. Then there are two possibilities: either that LSN is on an
existing record boundary, or it isn't. In the former case, no aborted
contrecord record is required. In the latter case, we're writing at
LSN which would have been in the middle of a previous record, except
that the record in question was never finished or at least we don't
have all of it. So the first WAL we insert at our chosen starting LSN
must be an aborted contrecord record.
I'm not quite sure I understand the nature of the remaining bug here,
but this logic seems quite sketchy to me:
/*
* When not in standby mode we find that WAL ends in an incomplete
* record, keep track of that record. After recovery is done,
* we'll write a record to indicate to downstream WAL readers that
* that portion is to be ignored.
*/
if (!StandbyMode &&
!XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
{
abortedRecPtr = xlogreader->abortedRecPtr;
missingContrecPtr = xlogreader->missingContrecPtr;
}
I don't see what StandbyMode has to do with anything here. The
question is whether the place where we're going to begin writing WAL
is on an existing record boundary or not. If it so happens that when
StandbyMode is turned on we can never decide to promote in the middle
of a record, then maybe there's no need to track this information when
StandbyMode = true, but I don't see a good reason why we should make
that assumption. What if in the future somebody added a command that
says "don't keep trying to read more WAL, just promote RIGHT HERE?". I
think this logic would surely be incorrect in that case. It feels to
me like the right thing to do is to always keep track if WAL ends in
an incomplete record, and then when we promote, we write an aborted
contrecord record if WAL ended in an incomplete record, full stop.
Now, we do need to keep in mind that, while in StandbyMode, we might
reach the end of WAL more than once, because we have a polling loop
that looks for more WAL. So it does not work to just set the values
once and then assume that's the whole truth forever. But why not
handle that by storing the abortedRecPtr and missingContrecPtr
unconditionally after every call to XLogPrefetcherReadRecord()? They
will go back and forth between XLogRecPtrIsInvalid and other values
many times but the last value should -- I think -- be however things
ended up at the point where we decided to stop reading WAL.
I haven't really dived into this issue much so I might be totally off
base, but it just doesn't feel right to me that this should depend on
whether we're in standby mode. That seems at best incidentally related
to whether an aborted contrecord record is required.
P.S. "aborted contrecord record" is really quite an awkward turn of phrase!
--
Robert Haas
EDB: http://www.enterprisedb.com
Make COPY extendable in order to support Parquet and other formats
Hi hackers, In several conversations I had recently with colleagues it was pointed out that it would be great if PostgreSQL supported COPY to/from Parquet and other formats. I've found a corresponding discussion [1] on pgsql-general@. The consensus reached back in 2018 seems to be that this shouldn't be implemented in the core but rather an API should be provided for the extensions. To my knowledge this was never implemented though. I would like to invest some time into providing a corresponding patch for the core and implementing "pg_copy_parquet" extension as a practical example, and yet another, a bit simpler, extension as an API usage example for the core codebase. I just wanted to double-check that this is still a wanted feature and no one on pgsql-hackers@ objects the idea. Any feedback, suggestions and ideas are most welcome. [1]: https://postgr.es/m/20180210151304.fonjztsynewldfba%40gmail.com -- Best regards, Aleksander Alekseev
Re: Finer grain log timestamps
Alvaro Herrera writes: > Do we *have* to provide support for arbitrary numbers of digits, though? > We could provide support for only %.3t and %.6t specifically, and not > worry about other cases (error: width not supported). If I were coding it, I would allow only exactly 1 digit (%.Nt) to simplify the parsing side of things and bound the required buffer size. Without having written it, it's not clear to me whether further restricting the set of supported values would save much code. I will point out, though, that throwing an error during log_line_prefix processing will lead straight to infinite recursion. regards, tom lane
Re: Finer grain log timestamps
On 2022-Jun-14, David Fetter wrote: > On Mon, Jun 13, 2022 at 04:22:42PM -0400, Tom Lane wrote: > > A different line of thought is to extend %t to provide a precision > > field a la sprintf, so that for example "%.3t" is equivalent to > > "%m" and "%.6t" does what David wants, and we won't have to > > search for a new escape letter when the day arrives that > > somebody wants nanosecond resolution. The same could be done > > with %n, avoiding the need to find a different escape letter > > for that. > > I'll build this more sprintf-like thing if not doing so prevents the > change from happening, but frankly, I don't really see a point in it > because the next "log timestamps at some random negative power of 10 > second granularity" requirement I see will be the first. Do we *have* to provide support for arbitrary numbers of digits, though? We could provide support for only %.3t and %.6t specifically, and not worry about other cases (error: width not supported). When somebody wants %.9t in ten years, we won't have to fight for which letter to pick. And I agree that widening %m for everybody without recourse is not great. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: Skipping logical replication transactions on subscriber side
On Mon, Jun 20, 2022 at 9:52 AM Peter Eisentraut wrote: > That means changing the system's ABI, so in the extreme case you then > need to compile everything else to match as well. I think we wouldn't want to do that in a minor release, but doing it in a new major release seems fine -- especially if only AIX is affected. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Privileges on PUBLICATION
Euler Taveira wrote: > --eeab359ad6094efd84562cddd7fb9e89 > Content-Type: text/plain > > On Wed, May 18, 2022, at 6:44 AM, Antonin Houska wrote: > > ok, please see the next version. > The new paragraph looks good to me. I'm not sure if the CREATE PUBLICATION is > the right place to provide such information. As I suggested in a previous > email > [1], you could add it to "Logical Replication > Security". ok, I missed that. The next version moves the text there. > [1] https://postgr.es/m/d96103fe-99e2-4119-bd76-952d326b7...@www.fastmail.com -- Antonin Houska Web: https://www.cybertec-postgresql.com diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml index 145ea71d61b..2fcaa9d261a 100644 --- a/doc/src/sgml/logical-replication.sgml +++ b/doc/src/sgml/logical-replication.sgml @@ -1171,6 +1171,17 @@ CONTEXT: processing remote data for replication origin "pg_16395" during "INSER schema automatically, the user must be a superuser. + + Note that there are currently no privileges on publication, and that any + subscriber can access any publication. Thus if you're trying to hide some + information from particular subscribers (by using the + WHERE clause or the column list, or by not adding the + whole table to the publication), please be aware that other publications + can expose the same information. Publication privileges might be added + to PostgreSQL in the future to allow for + fine-grained access control. + + To create a subscription, the user must be a superuser.
Re: Skipping logical replication transactions on subscriber side
On 16.06.22 18:35, Robert Haas wrote: On Thu, Jun 16, 2022 at 3:26 AM Masahiko Sawada wrote: FWIW, looking at the manual, there might have been a solution for AIX to specify -qalign=natural compiler option in order to enforce the alignment of double to 8. Well if that can work it sure seems better. That means changing the system's ABI, so in the extreme case you then need to compile everything else to match as well.
Re: Add TAP test for auth_delay extension
On 22/06/18 12:07오후, Michael Paquier wrote:
> On Sat, Jun 18, 2022 at 11:06:02AM +0900, Dong Wook Lee wrote:
> > I have written a test for the auth_delay extension before,
> > but if it is okay, can you review it?
>
> +# check enter wrong password
> +my $t0 = [gettimeofday];
> +test_login($node, 'user_role', "wrongpass", 2);
> +my $elapsed = tv_interval($t0, [gettimeofday]);
> +ok($elapsed >= $delay_milliseconds / 1000, "auth_delay $elapsed seconds");
> +
> +# check enter correct password
> +my $t0 = [gettimeofday];
> +test_login($node, 'user_role', "pass", 0);
> +my $elapsed = tv_interval($t0, [gettimeofday]);
> +ok($elapsed < $delay_milliseconds / 1000, "auth_delay $elapsed seconds");
>
> On a slow machine, I suspect that the second test is going to be
> unstable as it would fail if the login attempt (that succeeds) takes
> more than $delay_milliseconds. You could increase more
> delay_milliseconds to leverage that, but it would make the first test
> slower for nothing on faster machines in the case where the
> authentication attempt has failed. I guess that you could leverage
> that by using a large value for delay_milliseconds in the second test,
> because we are never going to wait. For the first test, you could on
> the contrary use a much lower value, still on slow machines it may not
> test what the code path of auth_delay you are willing to test.
>
Thank you for your valuable advice I didn't think about the slow system.
Therefore, in the case of the second test, the time was extended a little.
> As a whole, I am not sure that this is really worth spending cycles on
> when running check-world or similar, and the code of the extension is
> trivial.
Even though it is trivial, I think it would be better if there was a test.
> --
> Michael
diff --git a/contrib/auth_delay/Makefile b/contrib/auth_delay/Makefile
index 4b86ec37f0..b65097789a 100644
--- a/contrib/auth_delay/Makefile
+++ b/contrib/auth_delay/Makefile
@@ -3,6 +3,8 @@
MODULES = auth_delay
PGFILEDESC = "auth_delay - delay authentication failure reports"
+TAP_TESTS = 1
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/auth_delay/t/001_auth_delay.pl b/contrib/auth_delay/t/001_auth_delay.pl
new file mode 100644
index 00..644026e4f2
--- /dev/null
+++ b/contrib/auth_delay/t/001_auth_delay.pl
@@ -0,0 +1,87 @@
+
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(gettimeofday tv_interval);
+
+# Delete pg_hba.conf from the given node, add a new entry to it
+# and then execute a reload to refresh it.
+sub reset_pg_hba
+{
+my $node = shift;
+my $hba_method = shift;
+
+unlink($node->data_dir . '/pg_hba.conf');
+# just for testing purposes, use a continuation line
+$node->append_conf('pg_hba.conf', "local all all $hba_method");
+$node->reload;
+return;
+}
+
+# Test access for a single role, with password
+sub test_login
+{
+local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+my $node = shift;
+my $role = shift;
+my $password = shift;
+my $expected_res = shift;
+my $status_string = 'failed';
+
+$status_string = 'success' if ($expected_res eq 0);
+
+my $connstr = "user=$role";
+my $testname =
+"authentication $status_string for role $role with password $password";
+
+$ENV{"PGPASSWORD"} = $password;
+
+if ($expected_res eq 0)
+{
+$node->connect_ok($connstr, $testname);
+}
+else
+{
+# No checks of the error message, only the status code.
+$node->connect_fails($connstr, $testname);
+}
+}
+
+note "setting up PostgreSQL instance";
+
+my $delay_milliseconds = 500;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+qq{shared_preload_libraries = 'auth_delay'
+ auth_delay.milliseconds = '$delay_milliseconds'});
+$node->start;
+
+$node->safe_psql(
+'postgres',
+"CREATE ROLE user_role LOGIN PASSWORD 'pass';");
+reset_pg_hba($node, 'password');
+
+note "running tests";
+
+# check enter wrong password
+my $t0 = [gettimeofday];
+test_login($node, 'user_role', "wrongpass", 2);
+my $elapsed = tv_interval($t0, [gettimeofday]);
+ok($elapsed >= $delay_milliseconds / 1000, "auth_delay $elapsed seconds");
+
+# check enter correct password
+my $t0 = [gettimeofday];
+test_login($node, 'user_role', "pass", 0);
+my $elapsed = tv_interval($t0, [gettimeofday]);
+ok($elapsed < $delay_milliseconds / 1000, "auth_delay $elapsed seconds");
+
+done_testing();
+
Re: SGML doc file references
On 17.06.22 21:33, Tom Lane wrote: Peter Eisentraut writes: On 17.06.22 19:52, Josh Soref wrote: ok, are they worth fixing? That would require renaming either the output files or the input files, and people would really not like either one. Agreed that renaming those files is not desirable, but the presented patch was only fixing erroneous/obsolete comments. Yeah, I had totally misinterpreted what was being proposed. Of course, the patch is most sensible. Committed.
Re: Handle infinite recursion in logical replication setup
On Mon, Jun 20, 2022 at 3:16 PM vignesh C wrote: > > On Mon, Jun 20, 2022 at 2:37 PM Dilip Kumar wrote: > > > > On Thu, Jun 16, 2022 at 3:48 PM vignesh C wrote: > > > > > > On Wed, Jun 15, 2022 at 12:09 PM Peter Smith > > > wrote: > > > > > > > > > Thanks for the comments, the attached v21 patch has the changes for the > > > same. > > > > I have done some basic review of v21 and I have a few comments, > > > > 1. > > +/* > > + * The subscription will request the publisher to only send changes that > > + * originated locally. > > + */ > > +#define LOGICALREP_ORIGIN_LOCAL "local" > > + > > +/* > > + * The subscription will request the publisher to send any changes > > regardless > > + * of their origin > > + */ > > +#define LOGICALREP_ORIGIN_ANY "any" > > > > Are we planning to extend this to more options or are we planning to > > support the actual origin name here? If not then why isn't it just > > bool? I think the comments and the patch commit message should > > explain the details behind it if it has been already discussed and > > concluded. > > Currently we only support local and any. But this was designed to > accept string instead of boolean type, so that it can be extended > later to support filtering of origin names specified by the user in > the later versions. The same was also discussed in pg unconference as > mentioned in [1]. I will add it to the commit message and a comment > for the same in the next version. Thanks for the comment, the v22 patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm1h-9UNi_Jo_K%2BPK34tXBmV7fhj5C_nB8YzGA9rmUwHEA%40mail.gmail.com Regards, Vignesh
Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
At Mon, 20 Jun 2022 16:13:43 +0900, Michael Paquier wrote
in
> On Fri, May 27, 2022 at 07:01:37PM +, Imseih (AWS), Sami wrote:
> > What we found:
> >
> > 1. missingContrecPtr is set when
> >StandbyMode is false, therefore
> >only a writer should set this value
> >and a record is then sent downstream.
> >
> >But a standby going through crash
> >recovery will always have StandbyMode = false,
> >causing the missingContrecPtr to be incorrectly
> >set.
>
> That stands as true as far as I know, StandbyMode would be switched
> only once we get out of crash recovery, and only if archive recovery
> completes when there is a restore_command.
Anyway the change;
- abortedRecPtr = InvalidXLogRecPtr;
- missingContrecPtr = InvalidXLogRecPtr;
+ //abortedRecPtr = InvalidXLogRecPtr;
+ //missingContrecPtr = InvalidXLogRecPtr;
Is injecting a bug that invalid "aborted contrecord" record can be
injected for certain conditions. If a bug is intentionally injected,
it's quite natrual that some behavior gets broken..
> > 2. If StandbyModeRequested is checked instead,
> > we ensure that a standby will not set a
> > missingContrecPtr.
> >
> > 3. After applying the patch below, the tap test succeeded
>
> Hmm. I have not looked at that in depth, but if the intention is to
> check that the database is able to write WAL, looking at
> XLogCtl->SharedRecoveryState would be the way to go because that's the
> flip switching between crash recovery, archive recovery and the end of
> recovery (when WAL can be safely written).
What we are checking there is "we are going to write WAL", not "we are
writing".
(!StanbyMode && StandbyModeRequested) means the server have been
running crash-recovery before starting archive recovery. In that
case, the server is supposed to continue with archived WAL without
insering a record. However, if no archived WAL available and the
server promoted, the server needs to insert an "aborted contrecord"
record again. (I'm not sure how that case happens in the field,
though.)
So I don't think !StandbyModeRequested is the right thing
here. Actually the attached test fails with the fix.
> The check in xlogrecovery_redo() still looks like a good thing to have
> anyway, because we know that we can safely skip the contrecord. Now,
> for any patch produced, could the existing TAP test be extended so as
> we are able to get a PANIC even if we keep around the sanity check in
> xlogrecovery_redo(). That would likely involve an immediate shutdown
> of a standby followed by a start sequence?
Thus, I still don't see what have happened at Imseih's hand, but I can
cause PANIC with a bit tricky steps, which I don't think valid. This
is what I wanted to know the exact steps to cause the PANIC.
The attached 1 is the PoC of the TAP test (it uses system()..), and
the second is a tentative fix for that. (I don't like the fix, too,
though...)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index 78feccd9aa..bce3d6d701 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -68,8 +68,7 @@ ok($initfile ne $endfile, "$initfile differs from $endfile");
# contents
$node->stop('immediate');
-unlink $node->basedir . "/pgdata/pg_wal/$endfile"
- or die "could not unlink " . $node->basedir . "/pgdata/pg_wal/$endfile: $!";
+system(sprintf("mv %s/pgdata/pg_wal/$endfile %s/archives/", $node->basedir, $node->basedir));
# OK, create a standby at this spot.
$node->backup_fs_cold('backup');
@@ -106,4 +105,17 @@ $node_standby->promote;
$node->stop;
$node_standby->stop;
+my $node_primary_2 = PostgreSQL::Test::Cluster->new('primary_2');
+$node_primary_2->init_from_backup($node, 'backup', has_restoring => 1, standby => 0);
+
+$node_primary_2->append_conf(
+ 'postgresql.conf', qq(
+log_min_messages = debug1
+));
+$node_primary_2->start;
+$node_primary_2->poll_query_until('postgres',
+ 'SELECT NOT pg_is_in_recovery()');
+is($node_primary_2->safe_psql('postgres', 'SELECT pg_is_in_recovery()'), 'f',
+ 'check if the copied server has promoted');
+
done_testing();
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 6eba626420..6b026af74e 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -3025,6 +3025,19 @@ ReadRecord(XLogPrefetcher *xlogprefetcher, int emode,
if (record)
{
+ /*
+ * If we see an complete record after recorded aborted contrecord,
+ * that means the aborted contrecord is a false one. This happens
+ * when we can continue archive recovery after crash recovery ended
+ * with aborted contrecord.
+ */
+ if (abortedRecPtr != InvalidXLogRecPtr &&
+abortedRecPtr <
Re: pltcl crash on recent macOS
On 14.06.22 05:05, Tom Lane wrote: I'd be okay with just dropping the -lc from pl/tcl/Makefile and seeing what the buildfarm says. The fact that we needed it in 1998 doesn't mean that we still need it on supported versions of Tcl; nor was it ever anything but a hack for us to be overriding what TCL_LIBS says. Ok, I propose to proceed with the attached patch (with a bit more explanation added) for the master branch (for now) and see how it goes.From 394ab358d7437768d2c2570381f2cdcef51dc2c4 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 20 Jun 2022 12:34:13 +0200 Subject: [PATCH] PL/Tcl: Don't link with -lc explicitly Discussion: https://www.postgresql.org/message-id/flat/a78c847a-4f79-9286-be99-e819e9e4139e%40enterprisedb.com --- src/pl/tcl/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pl/tcl/Makefile b/src/pl/tcl/Makefile index 25e65189b6..314f9b2eec 100644 --- a/src/pl/tcl/Makefile +++ b/src/pl/tcl/Makefile @@ -15,7 +15,7 @@ override CPPFLAGS := -I. -I$(srcdir) $(TCL_INCLUDE_SPEC) $(CPPFLAGS) # On Windows, we don't link directly with the Tcl library; see below ifneq ($(PORTNAME), win32) -SHLIB_LINK = $(TCL_LIB_SPEC) $(TCL_LIBS) -lc +SHLIB_LINK = $(TCL_LIB_SPEC) $(TCL_LIBS) endif PGFILEDESC = "PL/Tcl - procedural language" -- 2.36.1
Re: Handle infinite recursion in logical replication setup
On Mon, Jun 20, 2022 at 3:16 PM vignesh C wrote: > > On Mon, Jun 20, 2022 at 2:37 PM Dilip Kumar wrote: > > > > On Thu, Jun 16, 2022 at 3:48 PM vignesh C wrote: > > > > > > On Wed, Jun 15, 2022 at 12:09 PM Peter Smith > > > wrote: > > > > > > > > > Thanks for the comments, the attached v21 patch has the changes for the > > > same. > > > > I have done some basic review of v21 and I have a few comments, > > > > 1. > > +/* > > + * The subscription will request the publisher to only send changes that > > + * originated locally. > > + */ > > +#define LOGICALREP_ORIGIN_LOCAL "local" > > + > > +/* > > + * The subscription will request the publisher to send any changes > > regardless > > + * of their origin > > + */ > > +#define LOGICALREP_ORIGIN_ANY "any" > > > > Are we planning to extend this to more options or are we planning to > > support the actual origin name here? If not then why isn't it just > > bool? I think the comments and the patch commit message should > > explain the details behind it if it has been already discussed and > > concluded. > > Currently we only support local and any. But this was designed to > accept string instead of boolean type, so that it can be extended > later to support filtering of origin names specified by the user in > the later versions. The same was also discussed in pg unconference as > mentioned in [1]. I will add it to the commit message and a comment > for the same in the next version. > > > 2. > > +/* > > + * Check and throw an error if the publisher has subscribed to the same > > table > > + * from some other publisher. This check is required only if copydata is > > ON and > > + * the origin is local. > > + */ > > > > I think it should also explain why this combination is not allowed and > > if it is already explained in code > > then this code can add comments to refer to that part of the code. > > In the same function, the reason for this is mentioned detailly just > above the place where error is thrown. I think that should be enough. > Have a look and let me know if that is not sufficient: I think that should be sufficient, thanks. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Handle infinite recursion in logical replication setup
On Mon, Jun 20, 2022 at 2:37 PM Dilip Kumar wrote:
>
> On Thu, Jun 16, 2022 at 3:48 PM vignesh C wrote:
> >
> > On Wed, Jun 15, 2022 at 12:09 PM Peter Smith wrote:
> > >
>
> > Thanks for the comments, the attached v21 patch has the changes for the
> > same.
>
> I have done some basic review of v21 and I have a few comments,
>
> 1.
> +/*
> + * The subscription will request the publisher to only send changes that
> + * originated locally.
> + */
> +#define LOGICALREP_ORIGIN_LOCAL "local"
> +
> +/*
> + * The subscription will request the publisher to send any changes regardless
> + * of their origin
> + */
> +#define LOGICALREP_ORIGIN_ANY "any"
>
> Are we planning to extend this to more options or are we planning to
> support the actual origin name here? If not then why isn't it just
> bool? I think the comments and the patch commit message should
> explain the details behind it if it has been already discussed and
> concluded.
Currently we only support local and any. But this was designed to
accept string instead of boolean type, so that it can be extended
later to support filtering of origin names specified by the user in
the later versions. The same was also discussed in pg unconference as
mentioned in [1]. I will add it to the commit message and a comment
for the same in the next version.
> 2.
> +/*
> + * Check and throw an error if the publisher has subscribed to the same table
> + * from some other publisher. This check is required only if copydata is ON
> and
> + * the origin is local.
> + */
>
> I think it should also explain why this combination is not allowed and
> if it is already explained in code
> then this code can add comments to refer to that part of the code.
In the same function, the reason for this is mentioned detailly just
above the place where error is thrown. I think that should be enough.
Have a look and let me know if that is not sufficient:
+ /*
+* Throw an error if the publisher has subscribed to
the same table
+* from some other publisher. We cannot differentiate
between the
+* local and non-local data that is present in the
HEAP during the
+* initial sync. Identification of local data can be
done only from
+* the WAL by using the origin id. XXX: For
simplicity, we don't check
+* whether the table has any data or not. If the table
doesn't have
+* any data then we don't need to distinguish between local and
+* non-local data so we can avoid throwing error in that case.
+*/
+ if (!slot_attisnull(slot, 3))
+ ereport(ERROR,
+
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("table:%s.%s might have
replicated data in the publisher",
+ nspname, relname),
+ errdetail("CREATE/ALTER
SUBSCRIPTION with origin = local and copy_data = on is not allowed
when the publisher might have replicated data."),
+ errhint("Use CREATE/ALTER
SUBSCRIPTION with copy_data = off/force."));
[1] -
https://wiki.postgresql.org/wiki/PgCon_2022_Developer_Unconference#Logical_Replication_Origin_Filtering_and_Consistency
Regards,
Vignesh
Re: Handle infinite recursion in logical replication setup
On Thu, Jun 16, 2022 at 3:48 PM vignesh C wrote: > > On Wed, Jun 15, 2022 at 12:09 PM Peter Smith wrote: > > > Thanks for the comments, the attached v21 patch has the changes for the same. I have done some basic review of v21 and I have a few comments, 1. +/* + * The subscription will request the publisher to only send changes that + * originated locally. + */ +#define LOGICALREP_ORIGIN_LOCAL "local" + +/* + * The subscription will request the publisher to send any changes regardless + * of their origin + */ +#define LOGICALREP_ORIGIN_ANY "any" Are we planning to extend this to more options or are we planning to support the actual origin name here? If not then why isn't it just bool? I think the comments and the patch commit message should explain the details behind it if it has been already discussed and concluded. 2. +/* + * Check and throw an error if the publisher has subscribed to the same table + * from some other publisher. This check is required only if copydata is ON and + * the origin is local. + */ I think it should also explain why this combination is not allowed and if it is already explained in code then this code can add comments to refer to that part of the code. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths
On Mon, 20 Jun 2022 at 07:02, Michael Paquier wrote:
>
> On Sat, Jun 11, 2022 at 09:25:48PM +0200, Matthias van de Meent wrote:
> > Did you initiate a new cluster or otherwise skip the invalid record
> > you generated when running the instance based on master? It seems to
> > me you're trying to replay the invalid record (len > MaxAllocSize),
> > and this patch does not try to fix that issue. This patch just tries
> > to forbid emitting records larger than MaxAllocSize, as per the check
> > in XLogRecordAssemble, so that we wont emit unreadable records into
> > the WAL anymore.
> >
> > Reading unreadable records still won't be possible, but that's also
> > not something I'm trying to fix.
>
> As long as you cannot generate such WAL records that should be fine as
> wAL is not reused across upgrades, so this kind of restriction is a
> no-brainer on HEAD. The back-patching argument is not on the table
> anyway, as some of the routine signatures change with the unsigned
> arguments, because of those safety checks.
The signature change is mostly ornamental, see attached v4.backpatch.
The main reason for changing the signature is to make sure nobody can
provide a negative value, but it's not important to the patch.
>
> + if (unlikely(num_rdatas >= max_rdatas) ||
> + unlikely(!AllocSizeIsValid((uint64) mainrdata_len + (uint64) len)))
> + XLogErrorDataLimitExceeded();
> [...]
> +inline void
> +XLogErrorDataLimitExceeded()
> +{
> + elog(ERROR, "too much WAL data");
> +}
> The three checks are different, OK..
They each check slightly different things, but with the same error. In
RegisterData, it checks that the data can still be allocated and does
not overflow the register, in RegisterBlock it checks that the total
length of data registered to the block does not exceed the max value
of XLogRecordBlockHeader->data_length. I've updated the comments above
the checks so that this distinction is more clear.
> Note that static is missing.
Fixed in attached v4.patch
> + if (unlikely(!AllocSizeIsValid(total_len)))
> + XLogErrorDataLimitExceeded();
> Rather than a single check at the end of XLogRecordAssemble(), you'd
> better look after that each time total_len is added up?
I was doing so previously, but there were some good arguments against that:
- Performance of XLogRecordAssemble should be impacted as little as
possible. XLogRecordAssemble is in many hot paths, and it is highly
unlikely this check will be hit, because nobody else has previously
reported this issue. Any check, however unlikely, will add some
overhead, so removing check counts reduces overhead of this patch.
- The user or system is unlikely to care about which specific check
was hit, and only needs to care _that_ the check was hit. An attached
debugger will be able to debug the internals of the xlog machinery and
find out the specific reasons for the error, but I see no specific
reason why the specific reason would need to be reported to the
connection.
Kind regards,
Matthias van de Meent
v4-0001-Add-protections-in-xlog-record-APIs-against-large.patch
Description: Binary data
v4-0001-Add-protections-in-xlog-record-APIs-against-large.backpatch
Description: Binary data
Re: [PATCH] Completed unaccent dictionary with many missing characters
Michael Paquier wrote on 20.06.2022 03:49:
On Wed, Jun 15, 2022 at 01:01:37PM +0200, Przemysław Sztoch wrote:
Two fixes (bad comment and fixed Latin-ASCII.xml).
if codepoint.general_category.startswith('L') and \
- len(codepoint.combining_ids) > 1:
+ len(codepoint.combining_ids) > 0:
So, this one checks for the case where a codepoint is within the
letter category. As far as I can see this indeed adds a couple of
characters, with a combination of Greek and Latin letters. So that
looks fine.
Previously, there were only multi-letter conversions. Now we also have
single letters.
+elif codepoint.general_category.startswith('N') and \
+ len(codepoint.combining_ids) > 0 and \
+ args.noLigaturesExpansion is False and is_ligature(codepoint,
table):
+charactersSet.add((codepoint.id,
+ "".join(chr(combining_codepoint.id)
+ for combining_codepoint
+ in get_plain_letters(codepoint,
table
And this one is for the numerical part of the change. Do you actually
need to apply is_ligature() here? I would have thought that this only
applies to letters.
But ligature check is performed on combining_ids (result of
translation), not on base codepoint.
Without it, you will get assertions in get_plain_letters.
The idea is that we take translations that turn into normal letters.
Others (strange) are rejected.
Maybe it could be done better. I didn't like it as much as you did, but
I couldn't do better.
In the end, I left it just like in the original script.
Note that the plain letter list (PLAIN_LETTER_RANGES) has now been
expanded with numbers.
-assert(False)
+assert False, 'Codepoint U+%0.2X' % codepoint.id
[...]
-assert(is_ligature(codepoint, table))
+assert is_ligature(codepoint, table), 'Codepoint U+%0.2X' % codepoint.id
These two are a good idea for debugging.
-return all(is_letter(table[i], table) for i in codepoint.combining_ids)
+return all(i in table and is_letter(table[i], table) for i in
codepoint.combining_ids)
It looks like this makes the code weaker, as we would silently skip
characters that are not part of the table rather than checking for
them all the time?
Unfortunately, there are entries in combining_ids that are not in the
character table being used.
This protection is necessary so that there is no error. But unfamiliar
characters are omitted.
While recreating unaccent.rules with your patch, I have noticed what
looks like an error. An extra rule mapping U+210C (black-letter
capital h) to "x" gets added on top of te existing one for "H", but
the correct answer is the existing rule, not the one added by the
patch.
The problem with the sign of U+210C is that there are conflicting
translations for it.
As the name suggests "(black-letter capital h)", it should be converted
to a capital H.
However, the current Latin-ASCII.xml suggests a conversion to x.
I found an open discussion on the internet about this and the suggestion
that the Latin-ASCII.xml file should be corrected for this letter.
But I wouldn't expect that Unicode makes the revised Latin-ASCII.xml
quickly into the official repo.
--
Przemysław Sztoch | Mobile +48 509 99 00 66
Re: [BUG] Panic due to incorrect missingContrecPtr after promotion
On Fri, May 27, 2022 at 07:01:37PM +, Imseih (AWS), Sami wrote: > What we found: > > 1. missingContrecPtr is set when >StandbyMode is false, therefore >only a writer should set this value >and a record is then sent downstream. > >But a standby going through crash >recovery will always have StandbyMode = false, >causing the missingContrecPtr to be incorrectly >set. That stands as true as far as I know, StandbyMode would be switched only once we get out of crash recovery, and only if archive recovery completes when there is a restore_command. > 2. If StandbyModeRequested is checked instead, > we ensure that a standby will not set a > missingContrecPtr. > > 3. After applying the patch below, the tap test succeeded Hmm. I have not looked at that in depth, but if the intention is to check that the database is able to write WAL, looking at XLogCtl->SharedRecoveryState would be the way to go because that's the flip switching between crash recovery, archive recovery and the end of recovery (when WAL can be safely written). The check in xlogrecovery_redo() still looks like a good thing to have anyway, because we know that we can safely skip the contrecord. Now, for any patch produced, could the existing TAP test be extended so as we are able to get a PANIC even if we keep around the sanity check in xlogrecovery_redo(). That would likely involve an immediate shutdown of a standby followed by a start sequence? -- Michael signature.asc Description: PGP signature
RE: Replica Identity check of partition table on subscriber
On Mon, Jun 20, 2022 1:33 PM Amit Kapila wrote: > > On Fri, Jun 17, 2022 at 11:22 AM shiy.f...@fujitsu.com > wrote: > > > > On Fri Jun 17, 2022 11:06 AM shiy.f...@fujitsu.com > wrote: > > > > > > Attached the new version of patch set. I also moved the partitioned table > > > check > > > in logicalrep_rel_mark_updatable() to check_relation_updatable() as > > > discussed > > > [2]. > > > > > > > Attached back-branch patches of the first patch. > > > > One minor comment: > + /* > + * If it is a partitioned table, we don't check it, we will check its > + * partition later. > + */ > > Can we change the above comment to: "For partitioned tables, we only > need to care if the target partition is updatable (aka has PK or RI > defined for it)."? > Thanks for your comment. Modified in the attached patches. Regards, Shi yu v11-0001-Fix-partition-table-s-RI-checking-on-the-subscri.patch Description: v11-0001-Fix-partition-table-s-RI-checking-on-the-subscri.patch v11-pg13-0001-Fix-partition-table-s-RI-checking-on-the-subsc_patch Description: v11-pg13-0001-Fix-partition-table-s-RI-checking-on-the-subsc_patch v11-pg14-0001-Fix-partition-table-s-RI-checking-on-the-subsc_patch Description: v11-pg14-0001-Fix-partition-table-s-RI-checking-on-the-subsc_patch
Re: Add index scan progress to pg_stat_progress_vacuum
On Mon, Jun 6, 2022 at 11:42 PM Robert Haas wrote: > > On Thu, May 26, 2022 at 11:43 AM Masahiko Sawada > wrote: > > Another idea I came up with is that we can wait for all index vacuums > > to finish while checking and updating the progress information, and > > then calls WaitForParallelWorkersToFinish after confirming all index > > status became COMPLETED. That way, we don’t need to change the > > parallel query infrastructure. What do you think? > > +1 from me. It doesn't seem to me that we should need to add something > like parallel_vacuum_progress_callback in order to solve this problem, > because the parallel index vacuum code could just do the waiting > itself, as you propose here. > > The question Sami asks him his reply is a good one, though -- who is > to say that the leader only needs to update progress at the end, once > it's finished the index it's handling locally? There will need to be a > callback system of some kind to allow the leader to update progress as > other workers finish, even if the leader is still working. I am not > too sure that the idea of using the vacuum delay points is the best > plan. I think we should try to avoid piggybacking on such general > infrastructure if we can, and instead look for a way to tie this to > something that is specific to parallel vacuum. However, I haven't > studied the problem so I'm not sure whether there's a reasonable way > to do that. One idea would be to add a flag, say report_parallel_vacuum_progress, to IndexVacuumInfo struct and expect index AM to check and update the parallel index vacuum progress, say every 1GB blocks processed. The flag is true only when the leader process is vacuuming an index. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
