public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v9 2/5] Move code around in StartupXLOG(). 23+ messages / 5 participants [nested] [flat]
* [PATCH v9 2/5] Move code around in StartupXLOG(). @ 2021-09-16 08:07 Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 23+ messages in thread From: Heikki Linnakangas @ 2021-09-16 08:07 UTC (permalink / raw) This is the order that things will happen with the next commit, this makes it more explicit. To aid review, I added "BEGIN/END function" comments to mark which blocks of code are moved to separate functions in in the next commit. --- src/backend/access/transam/xlog.c | 469 ++++++++++++++++-------------- 1 file changed, 252 insertions(+), 217 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 61b79fed30f..472a88b1d40 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -892,7 +892,7 @@ static MemoryContext walDebugCxt = NULL; static void readRecoverySignalFile(void); static void validateRecoveryParameters(void); -static void exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog, +static void XLogInitNewTimeline(TimeLineID endTLI, XLogRecPtr endOfLog, TimeLineID newTLI); static void CleanupAfterArchiveRecovery(TimeLineID EndOfLogTLI, XLogRecPtr EndOfLog, @@ -5682,10 +5682,10 @@ validateRecoveryParameters(void) } /* - * Exit archive-recovery state + * Initialize the first WAL segment on new timeline. */ static void -exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog, TimeLineID newTLI) +XLogInitNewTimeline(TimeLineID endTLI, XLogRecPtr endOfLog, TimeLineID newTLI) { char xlogfname[MAXFNAMELEN]; XLogSegNo endLogSegNo; @@ -5694,26 +5694,11 @@ exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog, TimeLineID newTLI) /* we always switch to a new timeline after archive recovery */ Assert(endTLI != newTLI); - /* - * We are no longer in archive recovery state. - */ - InArchiveRecovery = false; - /* * Update min recovery point one last time. */ UpdateMinRecoveryPoint(InvalidXLogRecPtr, true); - /* - * If the ending log segment is still open, close it (to avoid problems on - * Windows with trying to rename or delete an open file). - */ - if (readFile >= 0) - { - close(readFile); - readFile = -1; - } - /* * Calculate the last segment on the old timeline, and the first segment * on the new timeline. If the switch happens in the middle of a segment, @@ -5770,19 +5755,6 @@ exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog, TimeLineID newTLI) */ XLogFileName(xlogfname, newTLI, startLogSegNo, wal_segment_size); XLogArchiveCleanup(xlogfname); - - /* - * Remove the signal files out of the way, so that we don't accidentally - * re-enter archive recovery mode in a subsequent crash. - */ - if (standby_signal_file_found) - durable_unlink(STANDBY_SIGNAL_FILE, FATAL); - - if (recovery_signal_file_found) - durable_unlink(RECOVERY_SIGNAL_FILE, FATAL); - - ereport(LOG, - (errmsg("archive recovery complete"))); } /* @@ -6682,11 +6654,12 @@ StartupXLOG(void) TimeLineID EndOfLogTLI; TimeLineID replayTLI, newTLI; + bool performedWalRecovery; + char *recoveryStopReason; XLogRecord *record; TransactionId oldestActiveXID; bool backupEndRequired = false; bool backupFromStandby = false; - DBState dbstate_at_startup; XLogReaderState *xlogreader; XLogPageReadPrivate private; bool promoted = false; @@ -6799,6 +6772,8 @@ StartupXLOG(void) SyncDataDirectory(); } + /*---- BEGIN InitWalRecovery ----*/ + /* * Initialize on the assumption we want to recover to the latest timeline * that's active according to pg_control. @@ -7055,20 +7030,6 @@ StartupXLOG(void) wasShutdown = ((record->xl_info & ~XLR_INFO_MASK) == XLOG_CHECKPOINT_SHUTDOWN); } - /* - * Clear out any old relcache cache files. This is *necessary* if we do - * any WAL replay, since that would probably result in the cache files - * being out of sync with database reality. In theory we could leave them - * in place if the database had been cleanly shut down, but it seems - * safest to just remove them always and let them be rebuilt during the - * first backend startup. These files needs to be removed from all - * directories including pg_tblspc, however the symlinks are created only - * after reading tablespace_map file in case of archive recovery from - * backup, so needs to clear old relcache files here after creating - * symlinks. - */ - RelationCacheInitFileRemove(); - /* * If the location of the checkpoint record is not on the expected * timeline in the history of the requested timeline, we cannot proceed: @@ -7131,9 +7092,112 @@ StartupXLOG(void) (errmsg_internal("commit timestamp Xid oldest/newest: %u/%u", checkPoint.oldestCommitTsXid, checkPoint.newestCommitTsXid))); + + /* sanity checks on the checkpoint record */ if (!TransactionIdIsNormal(XidFromFullTransactionId(checkPoint.nextXid))) ereport(PANIC, (errmsg("invalid next transaction ID"))); + if (checkPoint.redo > checkPointLoc) + ereport(PANIC, + (errmsg("invalid redo in checkpoint record"))); + + /* + * Check whether we need to force recovery from WAL. If it appears to + * have been a clean shutdown and we did not have a recovery signal file, + * then assume no recovery needed. + */ + if (checkPoint.redo < checkPointLoc) + { + if (wasShutdown) + ereport(PANIC, + (errmsg("invalid redo record in shutdown checkpoint"))); + InRecovery = true; + } + else if (ControlFile->state != DB_SHUTDOWNED) + InRecovery = true; + else if (ArchiveRecoveryRequested) + { + /* force recovery due to presence of recovery signal file */ + InRecovery = true; + } + + /* + * If recovery is needed, update our in-memory copy of pg_control to show + * that we are recovering and to show the selected checkpoint as the place + * we are starting from. We also mark pg_control with any minimum recovery + * stop point obtained from a backup history file. + * + * We don't write the changes to disk yet, though. Only do that after + * initializing various subsystems. + */ + if (InRecovery) + { + DBState dbstate_at_startup; + + dbstate_at_startup = ControlFile->state; + if (InArchiveRecovery) + { + ControlFile->state = DB_IN_ARCHIVE_RECOVERY; + } + else + { + ereport(LOG, + (errmsg("database system was not properly shut down; " + "automatic recovery in progress"))); + if (recoveryTargetTLI > ControlFile->checkPointCopy.ThisTimeLineID) + ereport(LOG, + (errmsg("crash recovery starts in timeline %u " + "and has target timeline %u", + ControlFile->checkPointCopy.ThisTimeLineID, + recoveryTargetTLI))); + ControlFile->state = DB_IN_CRASH_RECOVERY; + } + ControlFile->checkPoint = checkPointLoc; + ControlFile->checkPointCopy = checkPoint; + if (InArchiveRecovery) + { + /* initialize minRecoveryPoint if not set yet */ + if (ControlFile->minRecoveryPoint < checkPoint.redo) + { + ControlFile->minRecoveryPoint = checkPoint.redo; + ControlFile->minRecoveryPointTLI = checkPoint.ThisTimeLineID; + } + } + + /* + * Set backupStartPoint if we're starting recovery from a base backup. + * + * Also set backupEndPoint and use minRecoveryPoint as the backup end + * location if we're starting recovery from a base backup which was + * taken from a standby. In this case, the database system status in + * pg_control must indicate that the database was already in recovery. + * Usually that will be DB_IN_ARCHIVE_RECOVERY but also can be + * DB_SHUTDOWNED_IN_RECOVERY if recovery previously was interrupted + * before reaching this point; e.g. because restore_command or + * primary_conninfo were faulty. + * + * Any other state indicates that the backup somehow became corrupted + * and we can't sensibly continue with recovery. + */ + if (haveBackupLabel) + { + ControlFile->backupStartPoint = checkPoint.redo; + ControlFile->backupEndRequired = backupEndRequired; + + if (backupFromStandby) + { + if (dbstate_at_startup != DB_IN_ARCHIVE_RECOVERY && + dbstate_at_startup != DB_SHUTDOWNED_IN_RECOVERY) + ereport(FATAL, + (errmsg("backup_label contains data inconsistent with control file"), + errhint("This means that the backup is corrupted and you will " + "have to use another backup for recovery."))); + ControlFile->backupEndPoint = ControlFile->minRecoveryPoint; + } + } + } + + /*---- END InitWalRecovery ----*/ /* initialize shared memory variables from the checkpoint record */ ShmemVariableCache->nextXid = checkPoint.nextXid; @@ -7147,6 +7211,20 @@ StartupXLOG(void) checkPoint.newestCommitTsXid); XLogCtl->ckptFullXid = checkPoint.nextXid; + /* + * Clear out any old relcache cache files. This is *necessary* if we do + * any WAL replay, since that would probably result in the cache files + * being out of sync with database reality. In theory we could leave them + * in place if the database had been cleanly shut down, but it seems + * safest to just remove them always and let them be rebuilt during the + * first backend startup. These files needs to be removed from all + * directories including pg_tblspc, however the symlinks are created only + * after reading tablespace_map file in case of archive recovery from + * backup, so needs to clear old relcache files here after creating + * symlinks. + */ + RelationCacheInitFileRemove(); + /* * Initialize replication slots, before there's a chance to remove * required resources. @@ -7231,30 +7309,6 @@ StartupXLOG(void) RedoRecPtr = XLogCtl->RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo; doPageWrites = lastFullPageWrites; - if (RecPtr < checkPoint.redo) - ereport(PANIC, - (errmsg("invalid redo in checkpoint record"))); - - /* - * Check whether we need to force recovery from WAL. If it appears to - * have been a clean shutdown and we did not have a recovery signal file, - * then assume no recovery needed. - */ - if (checkPoint.redo < RecPtr) - { - if (wasShutdown) - ereport(PANIC, - (errmsg("invalid redo record in shutdown checkpoint"))); - InRecovery = true; - } - else if (ControlFile->state != DB_SHUTDOWNED) - InRecovery = true; - else if (ArchiveRecoveryRequested) - { - /* force recovery due to presence of recovery signal file */ - InRecovery = true; - } - /* * Start recovery assuming that the final record isn't lost. */ @@ -7266,85 +7320,51 @@ StartupXLOG(void) { int rmid; + /* Initialize state for RecoveryInProgress() */ + SpinLockAcquire(&XLogCtl->info_lck); + if (InArchiveRecovery) + XLogCtl->SharedRecoveryState = RECOVERY_STATE_ARCHIVE; + else + XLogCtl->SharedRecoveryState = RECOVERY_STATE_CRASH; + SpinLockRelease(&XLogCtl->info_lck); + /* * Update pg_control to show that we are recovering and to show the * selected checkpoint as the place we are starting from. We also mark * pg_control with any minimum recovery stop point obtained from a * backup history file. + * + * No need to hold ControlFileLock yet, we aren't up far enough. */ - dbstate_at_startup = ControlFile->state; - if (InArchiveRecovery) - { - ControlFile->state = DB_IN_ARCHIVE_RECOVERY; - - SpinLockAcquire(&XLogCtl->info_lck); - XLogCtl->SharedRecoveryState = RECOVERY_STATE_ARCHIVE; - SpinLockRelease(&XLogCtl->info_lck); - } - else - { - ereport(LOG, - (errmsg("database system was not properly shut down; " - "automatic recovery in progress"))); - if (recoveryTargetTLI > ControlFile->checkPointCopy.ThisTimeLineID) - ereport(LOG, - (errmsg("crash recovery starts in timeline %u " - "and has target timeline %u", - ControlFile->checkPointCopy.ThisTimeLineID, - recoveryTargetTLI))); - ControlFile->state = DB_IN_CRASH_RECOVERY; + UpdateControlFile(); - SpinLockAcquire(&XLogCtl->info_lck); - XLogCtl->SharedRecoveryState = RECOVERY_STATE_CRASH; - SpinLockRelease(&XLogCtl->info_lck); - } - ControlFile->checkPoint = checkPointLoc; - ControlFile->checkPointCopy = checkPoint; - if (InArchiveRecovery) + /* + * If there was a backup label file, it's done its job and the info + * has now been propagated into pg_control. We must get rid of the + * label file so that if we crash during recovery, we'll pick up at + * the latest recovery restartpoint instead of going all the way back + * to the backup start point. It seems prudent though to just rename + * the file out of the way rather than delete it completely. + */ + if (haveBackupLabel) { - /* initialize minRecoveryPoint if not set yet */ - if (ControlFile->minRecoveryPoint < checkPoint.redo) - { - ControlFile->minRecoveryPoint = checkPoint.redo; - ControlFile->minRecoveryPointTLI = checkPoint.ThisTimeLineID; - } + unlink(BACKUP_LABEL_OLD); + durable_rename(BACKUP_LABEL_FILE, BACKUP_LABEL_OLD, FATAL); } /* - * Set backupStartPoint if we're starting recovery from a base backup. - * - * Also set backupEndPoint and use minRecoveryPoint as the backup end - * location if we're starting recovery from a base backup which was - * taken from a standby. In this case, the database system status in - * pg_control must indicate that the database was already in recovery. - * Usually that will be DB_IN_ARCHIVE_RECOVERY but also can be - * DB_SHUTDOWNED_IN_RECOVERY if recovery previously was interrupted - * before reaching this point; e.g. because restore_command or - * primary_conninfo were faulty. - * - * Any other state indicates that the backup somehow became corrupted - * and we can't sensibly continue with recovery. + * If there was a tablespace_map file, it's done its job and the + * symlinks have been created. We must get rid of the map file so + * that if we crash during recovery, we don't create symlinks again. + * It seems prudent though to just rename the file out of the way + * rather than delete it completely. */ - if (haveBackupLabel) + if (haveTblspcMap) { - ControlFile->backupStartPoint = checkPoint.redo; - ControlFile->backupEndRequired = backupEndRequired; - - if (backupFromStandby) - { - if (dbstate_at_startup != DB_IN_ARCHIVE_RECOVERY && - dbstate_at_startup != DB_SHUTDOWNED_IN_RECOVERY) - ereport(FATAL, - (errmsg("backup_label contains data inconsistent with control file"), - errhint("This means that the backup is corrupted and you will " - "have to use another backup for recovery."))); - ControlFile->backupEndPoint = ControlFile->minRecoveryPoint; - } + unlink(TABLESPACE_MAP_OLD); + durable_rename(TABLESPACE_MAP, TABLESPACE_MAP_OLD, FATAL); } - /* No need to hold ControlFileLock yet, we aren't up far enough */ - UpdateControlFile(); - /* * Initialize our local copy of minRecoveryPoint. When doing crash * recovery we want to replay up to the end of WAL. Particularly, in @@ -7371,33 +7391,6 @@ StartupXLOG(void) */ pgstat_reset_all(); - /* - * If there was a backup label file, it's done its job and the info - * has now been propagated into pg_control. We must get rid of the - * label file so that if we crash during recovery, we'll pick up at - * the latest recovery restartpoint instead of going all the way back - * to the backup start point. It seems prudent though to just rename - * the file out of the way rather than delete it completely. - */ - if (haveBackupLabel) - { - unlink(BACKUP_LABEL_OLD); - durable_rename(BACKUP_LABEL_FILE, BACKUP_LABEL_OLD, FATAL); - } - - /* - * If there was a tablespace_map file, it's done its job and the - * symlinks have been created. We must get rid of the map file so - * that if we crash during recovery, we don't create symlinks again. - * It seems prudent though to just rename the file out of the way - * rather than delete it completely. - */ - if (haveTblspcMap) - { - unlink(TABLESPACE_MAP_OLD); - durable_rename(TABLESPACE_MAP, TABLESPACE_MAP_OLD, FATAL); - } - /* Check that the GUCs used to generate the WAL allow recovery */ CheckRequiredParameterValues(); @@ -7481,12 +7474,7 @@ StartupXLOG(void) } } - /* Initialize resource managers */ - for (rmid = 0; rmid <= RM_MAX_ID; rmid++) - { - if (RmgrTable[rmid].rm_startup != NULL) - RmgrTable[rmid].rm_startup(); - } + /*---- BEGIN PerformWalRecovery ----*/ /* * Initialize shared variables for tracking progress of WAL replay, as @@ -7494,7 +7482,7 @@ StartupXLOG(void) * checkpoint record itself, if it's a shutdown checkpoint). */ SpinLockAcquire(&XLogCtl->info_lck); - if (checkPoint.redo < RecPtr) + if (checkPoint.redo < checkPointLoc) XLogCtl->replayEndRecPtr = checkPoint.redo; else XLogCtl->replayEndRecPtr = xlogreader->EndRecPtr; @@ -7526,7 +7514,7 @@ StartupXLOG(void) * Find the first record that logically follows the checkpoint --- it * might physically precede it, though. */ - if (checkPoint.redo < RecPtr) + if (checkPoint.redo < checkPointLoc) { /* back up to find the record */ XLogBeginRead(xlogreader, checkPoint.redo); @@ -7535,6 +7523,7 @@ StartupXLOG(void) else { /* just have to read next record after CheckPoint */ + Assert(RecPtr == checkPointLoc); record = ReadRecord(xlogreader, LOG, false, replayTLI); } @@ -7548,6 +7537,13 @@ StartupXLOG(void) InRedo = true; + /* Initialize resource managers */ + for (rmid = 0; rmid <= RM_MAX_ID; rmid++) + { + if (RmgrTable[rmid].rm_startup != NULL) + RmgrTable[rmid].rm_startup(); + } + ereport(LOG, (errmsg("redo starts at %X/%X", LSN_FORMAT_ARGS(xlogreader->ReadRecPtr)))); @@ -7850,8 +7846,13 @@ StartupXLOG(void) !reachedRecoveryTarget) ereport(FATAL, (errmsg("recovery ended before configured recovery target was reached"))); + + /*---- END PerformWalRecovery ----*/ + performedWalRecovery = true; } + /*---- BEGIN FinishWalRecovery ----*/ + /* * Kill WAL receiver, if it's still running, before we continue to write * the startup checkpoint and aborted-contrecord records. It will trump @@ -7860,23 +7861,6 @@ StartupXLOG(void) */ XLogShutdownWalRcv(); - /* - * Reset unlogged relations to the contents of their INIT fork. This is - * done AFTER recovery is complete so as to include any unlogged relations - * created during recovery, but BEFORE recovery is marked as having - * completed successfully. Otherwise we'd not retry if any of the post - * end-of-recovery steps fail. - */ - if (InRecovery) - ResetUnloggedRelations(UNLOGGED_RELATION_INIT); - - /* - * We don't need the latch anymore. It's not strictly necessary to disown - * it, but let's do it for the sake of tidiness. - */ - if (ArchiveRecoveryRequested) - DisownLatch(&XLogCtl->recoveryWakeupLatch); - /* * We are now done reading the xlog from stream. Turn off streaming * recovery to force fetching the files (which would be required at end of @@ -7909,6 +7893,32 @@ StartupXLOG(void) */ EndOfLogTLI = xlogreader->seg.ws_tli; + if (ArchiveRecoveryRequested) + { + /* + * We are no longer in archive recovery state. + * + * We are now done reading the old WAL. Turn off archive fetching if + * it was active. + */ + Assert(InArchiveRecovery); + InArchiveRecovery = false; + + /* + * If the ending log segment is still open, close it (to avoid problems on + * Windows with trying to rename or delete an open file). + */ + if (readFile >= 0) + { + close(readFile); + readFile = -1; + } + } + + recoveryStopReason = getRecoveryStopReason(); + + /*---- END FinishWalRecovery ----*/ + /* * Complain if we did not roll forward far enough to render the backup * dump consistent. Note: it is indeed okay to look at the local variable @@ -7945,6 +7955,16 @@ StartupXLOG(void) } } + /* + * Reset unlogged relations to the contents of their INIT fork. This is + * done AFTER recovery is complete so as to include any unlogged relations + * created during recovery, but BEFORE recovery is marked as having + * completed successfully. Otherwise we'd not retry if any of the post + * end-of-recovery steps fail. + */ + if (InRecovery) + ResetUnloggedRelations(UNLOGGED_RELATION_INIT); + /* * Pre-scan prepared transactions to find out the range of XIDs present. * This information is not quite needed yet, but it is positioned here so @@ -7953,8 +7973,8 @@ StartupXLOG(void) oldestActiveXID = PrescanPreparedTransactions(NULL, NULL); /* - * Allow ordinary WAL segment creation before any exitArchiveRecovery(), - * which sometimes creates a segment, and after the last ReadRecord(). + * Allow ordinary WAL segment creation before switching to a new timeline, + * which creates a new segment, and after the last ReadRecord(). */ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); XLogCtl->InstallXLogFileSegmentActive = true; @@ -7977,24 +7997,26 @@ StartupXLOG(void) newTLI = replayTLI; if (ArchiveRecoveryRequested) { - char *reason; - char recoveryPath[MAXPGPATH]; - - Assert(InArchiveRecovery); - newTLI = findNewestTimeLine(recoveryTargetTLI) + 1; ereport(LOG, (errmsg("selected new timeline ID: %u", newTLI))); - reason = getRecoveryStopReason(); + /* + * Make a writable copy of the last WAL segment. (Note that we also + * have a copy of the last block of the old WAL in readBuf; we will + * use that below.) + */ + XLogInitNewTimeline(EndOfLogTLI, EndOfLog, newTLI); /* - * We are now done reading the old WAL. Turn off archive fetching if - * it was active, and make a writable copy of the last WAL segment. - * (Note that we also have a copy of the last block of the old WAL in - * readBuf; we will use that below.) + * Remove the signal files out of the way, so that we don't accidentally + * re-enter archive recovery mode in a subsequent crash. */ - exitArchiveRecovery(EndOfLogTLI, EndOfLog, newTLI); + if (standby_signal_file_found) + durable_unlink(STANDBY_SIGNAL_FILE, FATAL); + + if (recovery_signal_file_found) + durable_unlink(RECOVERY_SIGNAL_FILE, FATAL); /* * Write the timeline history file, and have it archived. After this @@ -8007,18 +8029,10 @@ StartupXLOG(void) * between here and writing the end-of-recovery record. */ writeTimeLineHistory(newTLI, recoveryTargetTLI, - xlogreader->EndRecPtr, reason); - - /* - * Since there might be a partial WAL segment named RECOVERYXLOG, get - * rid of it. - */ - snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYXLOG"); - unlink(recoveryPath); /* ignore any error */ + EndOfLog, recoveryStopReason); - /* Get rid of any remaining recovered timeline-history file, too */ - snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYHISTORY"); - unlink(recoveryPath); /* ignore any error */ + ereport(LOG, + (errmsg("archive recovery complete"))); } /* Save the selected TimeLineID in shared memory, too */ @@ -8125,6 +8139,8 @@ StartupXLOG(void) /* Reload shared-memory state for prepared transactions */ RecoverPreparedTransactions(); + /*---- BEGIN ShutdownWalRecovery ----*/ + /* Shut down xlogreader */ if (readFile >= 0) { @@ -8133,6 +8149,31 @@ StartupXLOG(void) } XLogReaderFree(xlogreader); + if (ArchiveRecoveryRequested) + { + char recoveryPath[MAXPGPATH]; + + /* + * Since there might be a partial WAL segment named RECOVERYXLOG, get + * rid of it. + */ + snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYXLOG"); + unlink(recoveryPath); /* ignore any error */ + + /* Get rid of any remaining recovered timeline-history file, too */ + snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYHISTORY"); + unlink(recoveryPath); /* ignore any error */ + } + + /* + * We don't need the latch anymore. It's not strictly necessary to disown + * it, but let's do it for the sake of tidiness. + */ + if (ArchiveRecoveryRequested) + DisownLatch(&XLogCtl->recoveryWakeupLatch); + + /*---- END ShutdownWalRecovery ----*/ + /* Enable WAL writes for this backend only. */ LocalSetXLogInsertAllowed(); @@ -8155,14 +8196,8 @@ StartupXLOG(void) /* * Emit checkpoint or end-of-recovery record in XLOG, if required. - * - * XLogCtl->lastReplayedEndRecPtr will be a valid LSN if and only if we - * entered recovery. Even if we ultimately replayed no WAL records, it will - * have been initialized based on where replay was due to start. We don't - * need a lock to access this, since this can't change any more by the time - * we reach this code. */ - if (!XLogRecPtrIsInvalid(XLogCtl->lastReplayedEndRecPtr)) + if (performedWalRecovery) promoted = PerformRecoveryXLogAction(); /* -- 2.30.2 --------------24AFD7B565D1554637A9E916 Content-Type: text/x-patch; charset=UTF-8; name="v9-0003-Split-xlog.c-into-xlog.c-and-xlogrecovery.c.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="v9-0003-Split-xlog.c-into-xlog.c-and-xlogrecovery.c.patch" ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-03 04:33 Masahiko Sawada <[email protected]> 0 siblings, 2 replies; 23+ messages in thread From: Masahiko Sawada @ 2022-02-03 04:33 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Amit Kapila <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On Wed, Feb 2, 2022 at 4:36 PM David G. Johnston <[email protected]> wrote: > > On Tue, Feb 1, 2022 at 11:55 PM Amit Kapila <[email protected]> wrote: >> >> On Wed, Feb 2, 2022 at 9:41 AM David G. Johnston >> <[email protected]> wrote: >> > >> > On Tue, Feb 1, 2022 at 8:07 PM Amit Kapila <[email protected]> wrote: >> >> >> >> On Tue, Feb 1, 2022 at 11:47 AM Masahiko Sawada <[email protected]> wrote: >> >> >> >> > >> >> > I see that it's better to use a better IPC for ALTER SUBSCRIPTION SKIP >> >> > feature to pass error-XID or error-LSN information to the worker >> >> > whereas I'm also not sure of the advantages in storing all error >> >> > information in a system catalog. Since what we need to do for this >> >> > purpose is only error-XID/LSN, we can store only error-XID/LSN in the >> >> > catalog? That is, the worker stores error-XID/LSN in the catalog on an >> >> > error, and ALTER SUBSCRIPTION SKIP command enables the worker to skip >> >> > the transaction in question. The worker clears the error-XID/LSN after >> >> > successfully applying or skipping the first non-empty transaction. >> >> > >> >> >> >> Where do you propose to store this information? >> > >> > >> > pg_subscription_worker >> > >> > The error message and context is very important. Just make sure it is only non-null when the worker state is "syncing failed" (or whatever term we use). >> > >> > >> >> Sure, but is this the reason you want to store all the error info in >> the system catalog? I agree that providing more error info could be >> useful and also possibly the previously failed (apply) xacts info as >> well but I am not able to see why you want to have that sort of info >> in the catalog. I could see storing info like err_lsn/err_xid that can >> allow to proceed to apply worker automatically or to slow down the >> launch of errored apply worker but not all sort of other error info >> (like err_cnt, err_code, err_message, err_time, etc.). I want to know >> why you are insisting to make all the error info persistent via the >> system catalog? > > > I look at the catalog and am informed that the worker has stopped because of an error. I'd rather simply read the error message right then instead of having to go look at the log file. And if I am going to take an action in order to overcome the error I would have to know what that error is; so the error message is not something I can ignore. The error is an attribute of system state, and the catalog stores the current state of the (workers) system. > > I already explained that the concept of err_cnt is not useful. The fact that you include it here makes me think you are still thinking that this all somehow is meant to keep track of history. It is not. The workers are state machines and "error" is one of the states - with relevant attributes to display to the user, and system, while in that state. The state machine reporting does not care about historical states nor does it report on them. There is some uncertainty if we continue with the automatic re-launch; which, now that I write this, I can see where what you call err_cnt is effectively a count of how many times the worker re-launched without the underlying problem being resolved and thus encountered the same error. If we persist with the re-launch behavior then maybe err_cnt should be left in place - with the description for it basically being the ah-ha! comment I just made. In a world where we do not typically re-launch and simply re-try without being informed there is a change - such a count remains of minimal value. > > I don't really understand the confusion here though - this error data already exists in the pg_stat_subscription_workers stat collector view - the fact that I want to keep it around (just changing the reset behavior) - doesn't seem like it should be controversial. I, thinking as a user, really don't care about all of these implementation details. Whether it is a pg_stat_* view (collector or shmem IPC) or a pg_* catalog is immaterial to me. The behavior I observe is what matters. As a developer I don't want to use the statistics collector because these are not statistics and the collector is unreliable. I don't know enough about the relevant differences between shared memory IPC and catalog tables to decide between them. But catalog tables seem like a lower bar to meet and seem like they can implement the user-facing requirements as I envision them. I see that important information such as error-XID that can be used for ALTER SUBSCRIPTION SKIP needs to be stored in a reliable way, and using system catalogs is a reasonable way for this purpose. But it's still unclear to me why all error information that is currently shown in pg_stat_subscription_workers view, including error-XID and the error message, relation OID, action, etc., need to be stored in the catalog. The information other than error-XID doesn't necessarily need to be reliable compared to error-XID. I think we can have error-XID/LSN in the pg_subscription catalog and have other error information in pg_stat_subscription_workers view. After the user checks the current status of logical replication by checking error-XID/LSN, they can check pg_stat_subscription_workers for details. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-03 04:48 David G. Johnston <[email protected]> parent: Masahiko Sawada <[email protected]> 1 sibling, 0 replies; 23+ messages in thread From: David G. Johnston @ 2022-02-03 04:48 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Amit Kapila <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On Wednesday, February 2, 2022, Masahiko Sawada <[email protected]> wrote: > and have other error > information in pg_stat_subscription_workers view. > What benefit is there to keeping the existing collector-based pg_stat_subscripiton_workers view? If we re-write it using shmem IPC then we might as well put everything there and forego using a catalog. Then it behaves in a similar manner to pg_stat_activity but for logical replication workers. David J. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-15 18:26 Andres Freund <[email protected]> parent: Masahiko Sawada <[email protected]> 1 sibling, 1 reply; 23+ messages in thread From: Andres Freund @ 2022-02-15 18:26 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: David G. Johnston <[email protected]>; Amit Kapila <[email protected]>; pgsql-hackers Hi, On 2022-02-03 13:33:08 +0900, Masahiko Sawada wrote: > I see that important information such as error-XID that can be used > for ALTER SUBSCRIPTION SKIP needs to be stored in a reliable way, and > using system catalogs is a reasonable way for this purpose. But it's > still unclear to me why all error information that is currently shown > in pg_stat_subscription_workers view, including error-XID and the > error message, relation OID, action, etc., need to be stored in the > catalog. The information other than error-XID doesn't necessarily need > to be reliable compared to error-XID. I think we can have > error-XID/LSN in the pg_subscription catalog and have other error > information in pg_stat_subscription_workers view. After the user > checks the current status of logical replication by checking > error-XID/LSN, they can check pg_stat_subscription_workers for > details. The stats system isn't geared towards storing error messages and such. Generally it is about storing counts of events etc, not about information about a single event. Obviously there are a few cases where that boundary isn't that clear... IOW, storing information like: - subscription oid - retryable error count - hard error count - #replicated inserts - #replicated updates - #replicated deletes is what pgstats is for. But not - subscription oid - error message - xid of error - ... IMO the addition of the pg_stat_subscription_workers view needs to be reverted. Yes, there's some precedent in pg_stat_archiver. But that ship has sailed (it's released), and it's a much more limited issue. Just because we did a not great thing once isn't a reason to do a similar, but even less great, thing another time. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-16 08:49 Amit Kapila <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Amit Kapila @ 2022-02-16 08:49 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers On Tue, Feb 15, 2022 at 11:56 PM Andres Freund <[email protected]> wrote: > > On 2022-02-03 13:33:08 +0900, Masahiko Sawada wrote: > > I see that important information such as error-XID that can be used > > for ALTER SUBSCRIPTION SKIP needs to be stored in a reliable way, and > > using system catalogs is a reasonable way for this purpose. But it's > > still unclear to me why all error information that is currently shown > > in pg_stat_subscription_workers view, including error-XID and the > > error message, relation OID, action, etc., need to be stored in the > > catalog. The information other than error-XID doesn't necessarily need > > to be reliable compared to error-XID. I think we can have > > error-XID/LSN in the pg_subscription catalog and have other error > > information in pg_stat_subscription_workers view. After the user > > checks the current status of logical replication by checking > > error-XID/LSN, they can check pg_stat_subscription_workers for > > details. > > The stats system isn't geared towards storing error messages and > such. Generally it is about storing counts of events etc, not about > information about a single event. Obviously there are a few cases where that > boundary isn't that clear... > True, in the beginning, we discussed this information to be stored in system catalog [1] (See .... Also, I am thinking that instead of a stat view, do we need to consider having a system table .. ) but later discussion led us to store this as stats. > IOW, storing information like: > - subscription oid > - retryable error count > - hard error count > - #replicated inserts > - #replicated updates > - #replicated deletes > > is what pgstats is for. > Some of this and similar ((like error count, last_error_time)) is present in stats and something on the lines of the other information is proposed in [2] (xacts successfully replicated (committed), aborted, etc) to be stored along with it. > But not > - subscription oid > - error message > - xid of error > - ... > I think from the current set of things we are capturing, the other thing in this list will be error_command (insert/update/delete..) and or probably error_code. So, we can keep this information in a system table. Based on this discussion, it appears to me what we want here is to store the error info in some persistent way (system table) and the counters (both error and success) of logical replication in stats. If we can't achieve this work (separation) in the next few weeks (before the feature freeze) then I'll revert the work related to stats. [1] - https://www.postgresql.org/message-id/CAA4eK1LTE-AYtwatvLzAw%2BVy53C92QHoB7-rVbX-9nf8ws2Vag%40mail.g... [2] - https://commitfest.postgresql.org/37/3504/ -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-16 11:36 Masahiko Sawada <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Masahiko Sawada @ 2022-02-16 11:36 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Andres Freund <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers On Wed, Feb 16, 2022 at 5:49 PM Amit Kapila <[email protected]> wrote: > > On Tue, Feb 15, 2022 at 11:56 PM Andres Freund <[email protected]> wrote: > > > > On 2022-02-03 13:33:08 +0900, Masahiko Sawada wrote: > > > I see that important information such as error-XID that can be used > > > for ALTER SUBSCRIPTION SKIP needs to be stored in a reliable way, and > > > using system catalogs is a reasonable way for this purpose. But it's > > > still unclear to me why all error information that is currently shown > > > in pg_stat_subscription_workers view, including error-XID and the > > > error message, relation OID, action, etc., need to be stored in the > > > catalog. The information other than error-XID doesn't necessarily need > > > to be reliable compared to error-XID. I think we can have > > > error-XID/LSN in the pg_subscription catalog and have other error > > > information in pg_stat_subscription_workers view. After the user > > > checks the current status of logical replication by checking > > > error-XID/LSN, they can check pg_stat_subscription_workers for > > > details. > > > > The stats system isn't geared towards storing error messages and > > such. Generally it is about storing counts of events etc, not about > > information about a single event. Obviously there are a few cases where that > > boundary isn't that clear... > > > > True, in the beginning, we discussed this information to be stored in > system catalog [1] (See .... Also, I am thinking that instead of a > stat view, do we need to consider having a system table .. ) but later > discussion led us to store this as stats. > > > IOW, storing information like: > > - subscription oid > > - retryable error count > > - hard error count > > - #replicated inserts > > - #replicated updates > > - #replicated deletes > > > > is what pgstats is for. > > > > Some of this and similar ((like error count, last_error_time)) is > present in stats and something on the lines of the other information > is proposed in [2] (xacts successfully replicated (committed), > aborted, etc) to be stored along with it. > > > But not > > - subscription oid > > - error message > > - xid of error > > - ... > > > > I think from the current set of things we are capturing, the other > thing in this list will be error_command (insert/update/delete..) and > or probably error_code. So, we can keep this information in a system > table. Agreed. Also, we can have also commit-LSN or replace error-XID with error-LSN? > > Based on this discussion, it appears to me what we want here is to > store the error info in some persistent way (system table) and the > counters (both error and success) of logical replication in stats. If > we can't achieve this work (separation) in the next few weeks (before > the feature freeze) then I'll revert the work related to stats. There was an idea to use shmem to store error info but it seems to be better to use a system catalog to persist them. I'll summarize changes we discussed and make a plan of changes and feature designs toward the feature freeze (and v16). I think that once we get a consensus on them we can start implementation and move it forward. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-18 08:26 Masahiko Sawada <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 2 replies; 23+ messages in thread From: Masahiko Sawada @ 2022-02-18 08:26 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Andres Freund <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers On Wed, Feb 16, 2022 at 8:36 PM Masahiko Sawada <[email protected]> wrote: > > On Wed, Feb 16, 2022 at 5:49 PM Amit Kapila <[email protected]> wrote: > > > > On Tue, Feb 15, 2022 at 11:56 PM Andres Freund <[email protected]> wrote: > > > > > > On 2022-02-03 13:33:08 +0900, Masahiko Sawada wrote: > > > > I see that important information such as error-XID that can be used > > > > for ALTER SUBSCRIPTION SKIP needs to be stored in a reliable way, and > > > > using system catalogs is a reasonable way for this purpose. But it's > > > > still unclear to me why all error information that is currently shown > > > > in pg_stat_subscription_workers view, including error-XID and the > > > > error message, relation OID, action, etc., need to be stored in the > > > > catalog. The information other than error-XID doesn't necessarily need > > > > to be reliable compared to error-XID. I think we can have > > > > error-XID/LSN in the pg_subscription catalog and have other error > > > > information in pg_stat_subscription_workers view. After the user > > > > checks the current status of logical replication by checking > > > > error-XID/LSN, they can check pg_stat_subscription_workers for > > > > details. > > > > > > The stats system isn't geared towards storing error messages and > > > such. Generally it is about storing counts of events etc, not about > > > information about a single event. Obviously there are a few cases where that > > > boundary isn't that clear... > > > > > > > True, in the beginning, we discussed this information to be stored in > > system catalog [1] (See .... Also, I am thinking that instead of a > > stat view, do we need to consider having a system table .. ) but later > > discussion led us to store this as stats. > > > > > IOW, storing information like: > > > - subscription oid > > > - retryable error count > > > - hard error count > > > - #replicated inserts > > > - #replicated updates > > > - #replicated deletes > > > > > > is what pgstats is for. > > > > > > > Some of this and similar ((like error count, last_error_time)) is > > present in stats and something on the lines of the other information > > is proposed in [2] (xacts successfully replicated (committed), > > aborted, etc) to be stored along with it. > > > > > But not > > > - subscription oid > > > - error message > > > - xid of error > > > - ... > > > > > > > I think from the current set of things we are capturing, the other > > thing in this list will be error_command (insert/update/delete..) and > > or probably error_code. So, we can keep this information in a system > > table. > > Agreed. Also, we can have also commit-LSN or replace error-XID with error-LSN? > > > > > Based on this discussion, it appears to me what we want here is to > > store the error info in some persistent way (system table) and the > > counters (both error and success) of logical replication in stats. If > > we can't achieve this work (separation) in the next few weeks (before > > the feature freeze) then I'll revert the work related to stats. > > There was an idea to use shmem to store error info but it seems to be > better to use a system catalog to persist them. > > I'll summarize changes we discussed and make a plan of changes and > feature designs toward the feature freeze (and v16). I think that once > we get a consensus on them we can start implementation and move it > forward. > Here is the summary of the discussion, changes, and plan. 1. Move some error information such as the error message to a new system catalog, pg_subscription_error. The pg_subscription_error table would have the following columns: * sesubid : subscription Oid. * serelid : relation Oid (NULL for apply worker). * seerrlsn : commit-LSN or the error transaction. * seerrcmd : command (INSERT, UPDATE, etc.) of the error transaction. * seerrmsg : error message The tuple is inserted or updated when an apply worker or a tablesync worker raises an error. If the same error occurs in a row, the update is skipped. The tuple is removed in the following cases: * the subscription is dropped. * the table is dropped. * the table is removed from the subscription. * the worker successfully committed a non-empty transaction. With this change, pg_stat_subscription_workers will be like: * subid * subname * subrelid * error_count * last_error_timestamp This view will be extended by adding transaction statistics proposed on another thread[1]. 2. Fix a bug in pg_stat_subscription_workers. As pointed out by Andres, there is a bug in pg_stat_subscription_workers; it doesn't drop entries for already-dropped tables. We need to fix it. 3. Show commit-LSN of the error transaction in errcontext. Currently, we show XID and commit timestamp in the errcontext. But given that we use LSN in pg_subscriptoin_error, it's better to show commit-LSN as well (or instead of error-XID). 4. Skipping the particular conflicted transaction. There are two proposals: 4-1. Use the existing replication_origin_advance() SQL function. We don't need to add any additional syntax, instead use replication_origin_advance() with the error-LSN reported in either pg_subscription_error or server logs to skip the particular transaction. 4-2. Introduce a new syntax like ALTER SUBSCRIPTION ... SKIP. This proposal further has two options: (1) the user specifies error-LSN manually and (2) the user just enables skipping behavior and error-LSN is automatically fetched from pg_subscription_error. In any way, the command raises an error when there is no error entry in pg_subscription_error. We can discuss this item for details on the original thread. 5. Record skipped data to the system catalog, say pg_subscription_conflict_history so that there is a chance to analyze and recover. The pg_subscription_conflict_history I'm thinking is that we record the following of all skipped changes: * command (INSERT, UPDATE etc.) * commit-LSN * before row (in json format?) * after row (in json format?) Given that we end up writing a huge amount of history if the transaction is very large and I think there are peoples who want to check what changes will be skipped together before enabling skipping behavior, I think it could be optional. Therefore I think we can provide an option for ALTER SUBSCRIPTION ... SKIP to whether the skipped data is recorded or not and to the pg_subscription_conflict_history or server logs. We can discuss the details in a new thread. 4 and 5 might be better introduced together but I think since the user is able to check what changes will be skipped on the publisher side we can do 5 for v16. Feedback and comments are very welcome. Regards, [1] https://www.postgresql.org/message-id/flat/[email protected]... -- Masahiko Sawada EDB: https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-18 19:46 David G. Johnston <[email protected]> parent: Masahiko Sawada <[email protected]> 1 sibling, 3 replies; 23+ messages in thread From: David G. Johnston @ 2022-02-18 19:46 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Amit Kapila <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On Fri, Feb 18, 2022 at 1:26 AM Masahiko Sawada <[email protected]> wrote: > > Here is the summary of the discussion, changes, and plan. > > 1. Move some error information such as the error message to a new > system catalog, pg_subscription_error. The pg_subscription_error table > would have the following columns: > > * sesubid : subscription Oid. > * serelid : relation Oid (NULL for apply worker). > * seerrlsn : commit-LSN or the error transaction. > * seerrcmd : command (INSERT, UPDATE, etc.) of the error transaction. > * seerrmsg : error message > Not a fan of the "se" prefix but overall yes. We should include a timestamp. > The tuple is inserted or updated when an apply worker or a tablesync > worker raises an error. If the same error occurs in a row, the update > is skipped. I disagree with this - I would treat every new instance of an error to be important and insert on conflict (sesubid, serelid) the new entry, updating lsn/cmd/msg with the new values. The tuple is removed in the following cases: > > * the subscription is dropped. > * the table is dropped. * the table is removed from the subscription. > * the worker successfully committed a non-empty transaction. > Correct. This handles the "end of sync worker" just fine since its final action should be a successful commit of a non-empty transaction. > > With this change, pg_stat_subscription_workers will be like: > > * subid > * subname > * subrelid > * error_count > * last_error_timestamp > > This view will be extended by adding transaction statistics proposed > on another thread[1]. > I haven't reviewed that thread but in-so-far as this one goes I would just drop this altogether. The error count, if desired, can be added to pg_subscription_error, and the timestamp should be added there as noted above. If this is useful for the feature on the other thread it can be reconstituted from there. > 2. Fix a bug in pg_stat_subscription_workers. As pointed out by > Andres, there is a bug in pg_stat_subscription_workers; it doesn't > drop entries for already-dropped tables. We need to fix it. > Given the above, this becomes an N/A. > 3. Show commit-LSN of the error transaction in errcontext. Currently, > we show XID and commit timestamp in the errcontext. But given that we > use LSN in pg_subscriptoin_error, it's better to show commit-LSN as > well (or instead of error-XID). > Agreed, I think: what "errcontext" is this referring to? > > 5. Record skipped data to the system catalog, say > pg_subscription_conflict_history so that there is a chance to analyze > and recover. We can discuss the > details in a new thread. > Agreed - the "before skipping" consideration seems considerably more helpful; but wouldn't need to be persistent, it could just be a view. A permanent record probably would best be recorded in the logs - though if we get the pre-skip functionality the user can view directly and save the results wherever they wish or we can provide a function to spool the information to the log. I don't see persistent in-database storage being that desirable here. If we only do something after the transaction has been skipped it may be useful to add an option to the skipping command to auto-disable the subscription after the transaction has been skipped and before any subsequent transactions are applied. This will give the user a chance to process the post-skipped information before restoring sync and having the system begin changing underneath them again. > > 4 and 5 might be better introduced together but I think since the user > is able to check what changes will be skipped on the publisher side we > can do 5 for v16. How would one go about doing that (checking what changes will be skipped on the publisher side)? David J. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-18 20:32 Andres Freund <[email protected]> parent: Masahiko Sawada <[email protected]> 1 sibling, 1 reply; 23+ messages in thread From: Andres Freund @ 2022-02-18 20:32 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Amit Kapila <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers Hi, On 2022-02-18 17:26:04 +0900, Masahiko Sawada wrote: > With this change, pg_stat_subscription_workers will be like: > > * subid > * subname > * subrelid > * error_count > * last_error_timestamp > This view will be extended by adding transaction statistics proposed > on another thread[1]. I do not agree with these bits. What's the point of these per-relation stats at this poitns. You're just duplicating the normal relation pg_stats here. I really think we just should drop pg_stat_subscription_workers. Even if we don't, we definitely should rename it, because it still isn't meaningfully about workers. This stuff is getting painful for me. I'm trying to clean up some stuff for shared memory stats, and this stuff doesn't fit in with the rest. I'll have to rework some core stuff in the shared memory stats patch to make it work with this. Just to then quite possibly deal with reverting that part. Given the degree we're still designing stuff at this point, I think the appropriate thing is to revert the patch, and then try from there. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 10:49 Amit Kapila <[email protected]> parent: David G. Johnston <[email protected]> 2 siblings, 1 reply; 23+ messages in thread From: Amit Kapila @ 2022-02-19 10:49 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On Sat, Feb 19, 2022 at 1:17 AM David G. Johnston <[email protected]> wrote: > > On Fri, Feb 18, 2022 at 1:26 AM Masahiko Sawada <[email protected]> wrote: >> >> >> Here is the summary of the discussion, changes, and plan. >> >> 1. Move some error information such as the error message to a new >> system catalog, pg_subscription_error. The pg_subscription_error table >> would have the following columns: >> >> * sesubid : subscription Oid. >> * serelid : relation Oid (NULL for apply worker). >> * seerrlsn : commit-LSN or the error transaction. >> * seerrcmd : command (INSERT, UPDATE, etc.) of the error transaction. >> * seerrmsg : error message > > > Not a fan of the "se" prefix but overall yes. We should include a timestamp. > How about naming it pg_subscription_worker_error as Peter E. has suggested in one of his emails? I find pg_subscription_error slightly odd as one could imagine that even the errors related to subscription commands like Alter Subscription. >> >> The tuple is inserted or updated when an apply worker or a tablesync >> worker raises an error. If the same error occurs in a row, the update >> is skipped. > Are you going to query table to check if it is same error? > > I disagree with this - I would treat every new instance of an error to be important and insert on conflict (sesubid, serelid) the new entry, updating lsn/cmd/msg with the new values. > I don't think that will be a problem but what advantage are you envisioning with updating the same info except for timestamp? >> The tuple is removed in the following cases: >> >> * the subscription is dropped. >> * the table is dropped. >> >> * the table is removed from the subscription. >> * the worker successfully committed a non-empty transaction. > > > Correct. This handles the "end of sync worker" just fine since its final action should be a successful commit of a non-empty transaction. >> I think for tablesync workers, we may need slightly different handling as there could probably be no transactions to apply apart from the initial copy. Now, I think for tablesync worker, we can't postpone it till after we update the rel state as SUBREL_STATE_SYNCDONE because if we do it after that and there is some error updating/deleting the tuple, the tablesync worker won't be launched again and that entry will remain in the system for a longer duration. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 13:21 David G. Johnston <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: David G. Johnston @ 2022-02-19 13:21 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On Saturday, February 19, 2022, Amit Kapila <[email protected]> wrote: > On Sat, Feb 19, 2022 at 1:17 AM David G. Johnston > <[email protected]> wrote: > > > > On Fri, Feb 18, 2022 at 1:26 AM Masahiko Sawada <[email protected]> > wrote: > >> > >> > >> Here is the summary of the discussion, changes, and plan. > >> > >> 1. Move some error information such as the error message to a new > >> system catalog, pg_subscription_error. The pg_subscription_error table > >> would have the following columns: > >> > >> * sesubid : subscription Oid. > >> * serelid : relation Oid (NULL for apply worker). > >> * seerrlsn : commit-LSN or the error transaction. > >> * seerrcmd : command (INSERT, UPDATE, etc.) of the error transaction. > >> * seerrmsg : error message > > > > > > Not a fan of the "se" prefix but overall yes. We should include a > timestamp. > > > > How about naming it pg_subscription_worker_error as Peter E. has > suggested in one of his emails? I find pg_subscription_error slightly > odd as one could imagine that even the errors related to subscription > commands like Alter Subscription. > > Adding worker makes sense. > >> > >> The tuple is inserted or updated when an apply worker or a tablesync > >> worker raises an error. If the same error occurs in a row, the update > >> is skipped. > > > > Are you going to query table to check if it is same error? I don’t get the question, the quoted text is your which I disagree with. But the error message is being captured in any case. > > > > > I disagree with this - I would treat every new instance of an error to > be important and insert on conflict (sesubid, serelid) the new entry, > updating lsn/cmd/msg with the new values. > > > > I don't think that will be a problem but what advantage are you > envisioning with updating the same info except for timestamp? Omission of timestamp (or any other non-key field we have) from the update is an oversight. > >> The tuple is removed in the following cases: > >> > >> * the subscription is dropped. > >> * the table is dropped. > >> > >> * the table is removed from the subscription. > >> * the worker successfully committed a non-empty transaction. > > > > > > Correct. This handles the "end of sync worker" just fine since its > final action should be a successful commit of a non-empty transaction. > >> > > I think for tablesync workers, we may need slightly different handling > as there could probably be no transactions to apply apart from the > initial copy. Now, I think for tablesync worker, we can't postpone it > till after we update the rel state as SUBREL_STATE_SYNCDONE because if > we do it after that and there is some error updating/deleting the > tuple, the tablesync worker won't be launched again and that entry > will remain in the system for a longer duration. > Not sure…but I don’t see how you can not have a non-empty transaction while still having an error. Are subscriptions “dropped” upon a reboot? If not, that needs its own case for row removal. David J. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 13:36 Amit Kapila <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Amit Kapila @ 2022-02-19 13:36 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On Sat, Feb 19, 2022 at 6:51 PM David G. Johnston <[email protected]> wrote: > > On Saturday, February 19, 2022, Amit Kapila <[email protected]> wrote: >> >> On Sat, Feb 19, 2022 at 1:17 AM David G. Johnston >> <[email protected]> wrote: >> > >> > On Fri, Feb 18, 2022 at 1:26 AM Masahiko Sawada <[email protected]> wrote: >> >> >> >> >> >> Here is the summary of the discussion, changes, and plan. >> >> >> >> 1. Move some error information such as the error message to a new >> >> system catalog, pg_subscription_error. The pg_subscription_error table >> >> would have the following columns: >> >> >> >> * sesubid : subscription Oid. >> >> * serelid : relation Oid (NULL for apply worker). >> >> * seerrlsn : commit-LSN or the error transaction. >> >> * seerrcmd : command (INSERT, UPDATE, etc.) of the error transaction. >> >> * seerrmsg : error message >> > >> > >> > Not a fan of the "se" prefix but overall yes. We should include a timestamp. >> > >> >> How about naming it pg_subscription_worker_error as Peter E. has >> suggested in one of his emails? I find pg_subscription_error slightly >> odd as one could imagine that even the errors related to subscription >> commands like Alter Subscription. >> > > Adding worker makes sense. > >> >> >> >> >> The tuple is inserted or updated when an apply worker or a tablesync >> >> worker raises an error. If the same error occurs in a row, the update >> >> is skipped. >> > >> >> Are you going to query table to check if it is same error? > > > I don’t get the question, the quoted text is your which I disagree with. > It was Sawada-San's email and this question was for him. > But the error message is being captured in any case. >> >> >> > >> > I disagree with this - I would treat every new instance of an error to be important and insert on conflict (sesubid, serelid) the new entry, updating lsn/cmd/msg with the new values. >> > >> >> I don't think that will be a problem but what advantage are you >> envisioning with updating the same info except for timestamp? > > > Omission of timestamp (or any other non-key field we have) from the update is an oversight. > Yeah, if we decide to keep timestamp which the original proposal doesn't have then it makes sense to update again. >> >> >> The tuple is removed in the following cases: >> >> >> >> * the subscription is dropped. >> >> * the table is dropped. >> >> >> >> * the table is removed from the subscription. >> >> * the worker successfully committed a non-empty transaction. >> > >> > >> > Correct. This handles the "end of sync worker" just fine since its final action should be a successful commit of a non-empty transaction. >> >> >> >> I think for tablesync workers, we may need slightly different handling >> as there could probably be no transactions to apply apart from the >> initial copy. Now, I think for tablesync worker, we can't postpone it >> till after we update the rel state as SUBREL_STATE_SYNCDONE because if >> we do it after that and there is some error updating/deleting the >> tuple, the tablesync worker won't be launched again and that entry >> will remain in the system for a longer duration. > > > Not sure…but I don’t see how you can not have a non-empty transaction while still having an error. > > Are subscriptions “dropped” upon a reboot? If not, that needs its own case for row removal. > Subscriptions are not dropped automatically on reboot but I don't understand what you mean by "that needs its own case for row removal"? -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 13:38 Masahiko Sawada <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Masahiko Sawada @ 2022-02-19 13:38 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Amit Kapila <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers On Sat, Feb 19, 2022 at 5:32 AM Andres Freund <[email protected]> wrote: > > Hi, > > On 2022-02-18 17:26:04 +0900, Masahiko Sawada wrote: > > With this change, pg_stat_subscription_workers will be like: > > > > * subid > > * subname > > * subrelid > > * error_count > > * last_error_timestamp > > > This view will be extended by adding transaction statistics proposed > > on another thread[1]. > > I do not agree with these bits. What's the point of these per-relation stats > at this poitns. You're just duplicating the normal relation pg_stats here. > > I really think we just should drop pg_stat_subscription_workers. Even if we > don't, we definitely should rename it, because it still isn't meaningfully > about workers. The view has stats per subscription worker (i.e., apply worker and tablesync worker), not per relation. The subrelid is OID of the relation that the tablesync worker is synchronizing. For the stats of apply workers, it is null. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 13:43 Amit Kapila <[email protected]> parent: David G. Johnston <[email protected]> 2 siblings, 0 replies; 23+ messages in thread From: Amit Kapila @ 2022-02-19 13:43 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On Sat, Feb 19, 2022 at 1:17 AM David G. Johnston <[email protected]> wrote: > > On Fri, Feb 18, 2022 at 1:26 AM Masahiko Sawada <[email protected]> wrote: >> >> 5. Record skipped data to the system catalog, say >> pg_subscription_conflict_history so that there is a chance to analyze >> and recover. > > >> We can discuss the >> details in a new thread. > > Agreed - the "before skipping" consideration seems considerably more helpful; but wouldn't need to be persistent, it could just be a view. A permanent record probably would best be recorded in the logs - though if we get the pre-skip functionality the user can view directly and save the results wherever they wish or we can provide a function to spool the information to the log. I don't see persistent in-database storage being that desirable here. > We can consider this but note that it could fill up a lot of LOG and difficult to find/interpret information. Say after skipping and logging half of the transaction data, there is some error like "connection error", it will then repeat the entire table data again. Also, say the table has toast columns, we have some mechanism to write such data in tables (like by compressing, etc) but printing huge data in Logs would be tricky and it may not be easier to read it, we may even need some sort of tuple header, column header etc. Also, there could be errors from other sessions in-between which we should be able to filter out but still it's may not be that clear. > If we only do something after the transaction has been skipped it may be useful to add an option to the skipping command to auto-disable the subscription after the transaction has been skipped and before any subsequent transactions are applied. This will give the user a chance to process the post-skipped information before restoring sync and having the system begin changing underneath them again. > I think it can be helpful and probably can be done as a separate patch? -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 13:44 Masahiko Sawada <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 0 replies; 23+ messages in thread From: Masahiko Sawada @ 2022-02-19 13:44 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: David G. Johnston <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On Sat, Feb 19, 2022 at 10:37 PM Amit Kapila <[email protected]> wrote: > > On Sat, Feb 19, 2022 at 6:51 PM David G. Johnston > <[email protected]> wrote: > > > > On Saturday, February 19, 2022, Amit Kapila <[email protected]> wrote: > >> > >> On Sat, Feb 19, 2022 at 1:17 AM David G. Johnston > >> <[email protected]> wrote: > >> > > >> > On Fri, Feb 18, 2022 at 1:26 AM Masahiko Sawada <[email protected]> wrote: > >> >> > >> >> > >> >> Here is the summary of the discussion, changes, and plan. > >> >> > >> >> 1. Move some error information such as the error message to a new > >> >> system catalog, pg_subscription_error. The pg_subscription_error table > >> >> would have the following columns: > >> >> > >> >> * sesubid : subscription Oid. > >> >> * serelid : relation Oid (NULL for apply worker). > >> >> * seerrlsn : commit-LSN or the error transaction. > >> >> * seerrcmd : command (INSERT, UPDATE, etc.) of the error transaction. > >> >> * seerrmsg : error message > >> > > >> > > >> > Not a fan of the "se" prefix but overall yes. We should include a timestamp. > >> > > >> > >> How about naming it pg_subscription_worker_error as Peter E. has > >> suggested in one of his emails? I find pg_subscription_error slightly > >> odd as one could imagine that even the errors related to subscription > >> commands like Alter Subscription. > >> > > > > Adding worker makes sense. Agreed. > > > >> > >> >> > >> >> The tuple is inserted or updated when an apply worker or a tablesync > >> >> worker raises an error. If the same error occurs in a row, the update > >> >> is skipped. > >> > > >> > >> Are you going to query table to check if it is same error? > > > > > > I don’t get the question, the quoted text is your which I disagree with. > > > > It was Sawada-San's email and this question was for him. What I wanted to say about how to insert/update the tuple to pg_subscription_worker_error is that we insert a new tuple for the first time, then, when the next error occurred, the worker fetches the tuple and checks if the error (i.e., error-LSN, error-cmd, and error-message) is exactly the same as previous one. If it is, we can skip updating it. But if we include the error-timestamp in the tuple, we need to update it every time. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 13:48 Masahiko Sawada <[email protected]> parent: David G. Johnston <[email protected]> 2 siblings, 0 replies; 23+ messages in thread From: Masahiko Sawada @ 2022-02-19 13:48 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Amit Kapila <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers On Sat, Feb 19, 2022 at 4:47 AM David G. Johnston <[email protected]> wrote: > > On Fri, Feb 18, 2022 at 1:26 AM Masahiko Sawada <[email protected]> wrote: >> >> >> Here is the summary of the discussion, changes, and plan. >> >> 1. Move some error information such as the error message to a new >> system catalog, pg_subscription_error. The pg_subscription_error table >> would have the following columns: >> >> * sesubid : subscription Oid. >> * serelid : relation Oid (NULL for apply worker). >> * seerrlsn : commit-LSN or the error transaction. >> * seerrcmd : command (INSERT, UPDATE, etc.) of the error transaction. >> * seerrmsg : error message > > > Not a fan of the "se" prefix but overall yes. We should include a timestamp. > >> >> The tuple is inserted or updated when an apply worker or a tablesync >> worker raises an error. If the same error occurs in a row, the update >> is skipped. > > > I disagree with this - I would treat every new instance of an error to be important and insert on conflict (sesubid, serelid) the new entry, updating lsn/cmd/msg with the new values. > >> The tuple is removed in the following cases: >> >> * the subscription is dropped. >> * the table is dropped. >> >> * the table is removed from the subscription. >> * the worker successfully committed a non-empty transaction. > > > Correct. This handles the "end of sync worker" just fine since its final action should be a successful commit of a non-empty transaction. >> >> >> With this change, pg_stat_subscription_workers will be like: >> >> * subid >> * subname >> * subrelid >> * error_count >> * last_error_timestamp >> >> This view will be extended by adding transaction statistics proposed >> on another thread[1]. > > > I haven't reviewed that thread but in-so-far as this one goes I would just drop this altogether. The error count, if desired, can be added to pg_subscription_error, and the timestamp should be added there as noted above. > > If this is useful for the feature on the other thread it can be reconstituted from there. > >> >> 2. Fix a bug in pg_stat_subscription_workers. As pointed out by >> Andres, there is a bug in pg_stat_subscription_workers; it doesn't >> drop entries for already-dropped tables. We need to fix it. > > > Given the above, this becomes an N/A. > >> >> 3. Show commit-LSN of the error transaction in errcontext. Currently, >> we show XID and commit timestamp in the errcontext. But given that we >> use LSN in pg_subscriptoin_error, it's better to show commit-LSN as >> well (or instead of error-XID). > > > Agreed, I think: what "errcontext" is this referring to? The CONTEXT part in a log message. The apply worker and the tablesync worker write the details of the changes on an error in the CONTEXT part as follow: ERROR: duplicate key value violates unique constraint "test_pkey" DETAIL: Key (id)=(1) already exists. CONTEXT: processing remote data during "INSERT" for replication target relation "public.test" in transaction 716 with commit timestamp 2021-09-29 15:52:45.165754+00 >> >> >> 5. Record skipped data to the system catalog, say >> pg_subscription_conflict_history so that there is a chance to analyze >> and recover. > > >> We can discuss the >> details in a new thread. > > Agreed - the "before skipping" consideration seems considerably more helpful; but wouldn't need to be persistent, it could just be a view. A permanent record probably would best be recorded in the logs - though if we get the pre-skip functionality the user can view directly and save the results wherever they wish or we can provide a function to spool the information to the log. I don't see persistent in-database storage being that desirable here. > > If we only do something after the transaction has been skipped it may be useful to add an option to the skipping command to auto-disable the subscription after the transaction has been skipped and before any subsequent transactions are applied. This will give the user a chance to process the post-skipped information before restoring sync and having the system begin changing underneath them again. > >> >> >> 4 and 5 might be better introduced together but I think since the user >> is able to check what changes will be skipped on the publisher side we >> can do 5 for v16. > > > How would one go about doing that (checking what changes will be skipped on the publisher side)? We can copy the replication slot while changing the plugin by using pg_copy_replication_slot(). Then we can check the changes by using pg_logical_slot_peek_changes() with the copied slot. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 16:02 Andres Freund <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 2 replies; 23+ messages in thread From: Andres Freund @ 2022-02-19 16:02 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Amit Kapila <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers Hi, On 2022-02-19 22:38:19 +0900, Masahiko Sawada wrote: > On Sat, Feb 19, 2022 at 5:32 AM Andres Freund <[email protected]> wrote: > > On 2022-02-18 17:26:04 +0900, Masahiko Sawada wrote: > > > With this change, pg_stat_subscription_workers will be like: > > > > > > * subid > > > * subname > > > * subrelid > > > * error_count > > > * last_error_timestamp > > > > > This view will be extended by adding transaction statistics proposed > > > on another thread[1]. > > > > I do not agree with these bits. What's the point of these per-relation stats > > at this poitns. You're just duplicating the normal relation pg_stats here. > > > > I really think we just should drop pg_stat_subscription_workers. Even if we > > don't, we definitely should rename it, because it still isn't meaningfully > > about workers. > > The view has stats per subscription worker (i.e., apply worker and > tablesync worker), not per relation. The subrelid is OID of the > relation that the tablesync worker is synchronizing. For the stats of > apply workers, it is null. That's precisely the misuse of the stats subsystem that I'm complaining about here. The whole design of pgstat (as it is today) only makes sense if you can loose a message and it doesn't matter much, because it's just an incremental counter increment that's lost. And to be able properly prune dead pgstat contents the underlying objects stats are kept around either need to be permanent (e.g. stats about WAL) or a record of objects needs to exist (e.g. stats about relations). Even leaving everything else aside, a key of (dboid, subid, subrelid), where subrelid can be NULL, but where (dboid, subid) is *not* unique, imo is poor relational design. What is the justification for mixing relation specific and non-relation specific contents in this view? The patch you referenced [1] should just store the stats in the pg_stat_subscription view, not pg_stat_subscription_workers. It *does* make sense to keep stats about the number of table syncs that failed etc. But that should be a counter in pg_stat_subscription, not a row in pg_stat_subscription_workers. IOW, we should just drop pg_stat_subscription_workers, and add a few counters to pg_stat_subscription. Greetings, Andres Freund [1] https://www.postgresql.org/message-id/TYCPR01MB8373E658CEE48FE05505A120ED529%40TYCPR01MB8373.jpnprd0... ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 16:16 David G. Johnston <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 23+ messages in thread From: David G. Johnston @ 2022-02-19 16:16 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Amit Kapila <[email protected]>; pgsql-hackers On Sat, Feb 19, 2022 at 9:02 AM Andres Freund <[email protected]> wrote: > > Even leaving everything else aside, a key of (dboid, subid, subrelid), > where > subrelid can be NULL, but where (dboid, subid) is *not* unique, imo is poor > relational design. What is the justification for mixing relation specific > and > non-relation specific contents in this view? > The to-be-created pg_subscription_worker_error has this same design. It really is a worker-oriented view so the PK should ideally (ignoring dboid at the moment) be just workerid and subid and subrelid would just be attributes, of which subrelid is optional. But a worker's natural key is (subid, subrelid) so long as you accept that null has to be considered equal to itself. Not usually an ideal model to pick but it seems like this is one of those exceptions to the rule that works just fine. Feel free to use InvalidOID instead of null if that makes things more consistent and easier to implement. The conceptual model is still the same. It doesn't seem to be beneficial to have tablesync and apply workers have their own distinct relations. They are similar enough that they can share this one. > IOW, we should just drop pg_stat_subscription_workers, and add a few > counters > to pg_stat_subscription. > > +1 David J. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 16:37 Andres Freund <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Andres Freund @ 2022-02-19 16:37 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Amit Kapila <[email protected]>; pgsql-hackers Hi, On 2022-02-19 09:16:40 -0700, David G. Johnston wrote: > On Sat, Feb 19, 2022 at 9:02 AM Andres Freund <[email protected]> wrote: > > Even leaving everything else aside, a key of (dboid, subid, subrelid), > > where > > subrelid can be NULL, but where (dboid, subid) is *not* unique, imo is poor > > relational design. What is the justification for mixing relation specific > > and > > non-relation specific contents in this view? > The to-be-created pg_subscription_worker_error has this same design. > It really is a worker-oriented view so the PK should ideally (ignoring > dboid at the moment) be just workerid and subid and subrelid would just be > attributes, of which subrelid is optional. But a worker's natural key is > (subid, subrelid) so long as you accept that null has to be considered > equal to itself. Not usually an ideal model to pick but it seems like this > is one of those exceptions to the rule that works just fine. Feel free to > use InvalidOID instead of null if that makes things more consistent and > easier to implement. The conceptual model is still the same. It doesn't > seem to be beneficial to have tablesync and apply workers have their own > distinct relations. They are similar enough that they can share this one. I'm not so convinced. "User space" reasonably might want to know why a certain replication hasn't yet become part of logical replication / why a refresh hasn't completed yet. For that something like SELECT * FROM pg_subscription ps JOIN pg_subscription_worker_error pwe ON (ps.oid = pwe.subid) WHERE subrelid = 'foo'::regclass; makes sense. The apply side is different from that. Yes, a "subrelid = ..." predicate would filter out non-relation specific failures, but it still seems weird that apply failures would even need to be filtered out for something like this. And conversely, monitoring for apply failures is conceptually looking for something different than tablesync. IMO the type of information you'd want for apply failures is substantially different enough from worker failures that I don't really see the temptation to put them in the same table. I also still think that _worker shouldn't be part of any of the naming here. It's an implementation detail that we use one worker for one tablesync etc. It'd make sense for one apply worker to sync multiple small tables, and it'd make a lot of sense for multiple apply workers to collaborate on syncing one large relation. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-19 17:04 David G. Johnston <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: David G. Johnston @ 2022-02-19 17:04 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Amit Kapila <[email protected]>; pgsql-hackers On Sat, Feb 19, 2022 at 9:37 AM Andres Freund <[email protected]> wrote: > IMO the type of information you'd want for apply failures is substantially > different enough from worker failures that I don't really see the temptation > to put them in the same table. > > It's an error message and a transaction LSN in both cases right now, along with knowledge of whether said transaction only affects a single table (relid is not null) or not (relid is null). Do you have a concrete idea in mind that would make this separation need more obvious? > I also still think that _worker shouldn't be part of any of the naming > here. It's an implementation detail that we use one worker for one > tablesync > etc. It'd make sense for one apply worker to sync multiple small tables, > and > it'd make a lot of sense for multiple apply workers to collaborate on > syncing > one large relation. > Good point. The existing design doesn't actually require the "worker status" concept I described; so let's not have worker be part of the name. So basically separate the proposed pg_subscription_error table into two: a pg_subscription_tablesync_error and pg_subscription_apply_error. The former having a relid field while the later does not. What fields belong on each? How about we have it both ways. Two tables but provide a canonical view union'ing them that a user can query to see whether any errors are presently affecting their system? David J. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-21 03:56 Masahiko Sawada <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 0 replies; 23+ messages in thread From: Masahiko Sawada @ 2022-02-21 03:56 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Amit Kapila <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers On Sun, Feb 20, 2022 at 1:02 AM Andres Freund <[email protected]> wrote: > > Hi, > > On 2022-02-19 22:38:19 +0900, Masahiko Sawada wrote: > > On Sat, Feb 19, 2022 at 5:32 AM Andres Freund <[email protected]> wrote: > > > On 2022-02-18 17:26:04 +0900, Masahiko Sawada wrote: > > > > With this change, pg_stat_subscription_workers will be like: > > > > > > > > * subid > > > > * subname > > > > * subrelid > > > > * error_count > > > > * last_error_timestamp > > > > > > > This view will be extended by adding transaction statistics proposed > > > > on another thread[1]. > > > > > > I do not agree with these bits. What's the point of these per-relation stats > > > at this poitns. You're just duplicating the normal relation pg_stats here. > > > > > > I really think we just should drop pg_stat_subscription_workers. Even if we > > > don't, we definitely should rename it, because it still isn't meaningfully > > > about workers. > > > > The view has stats per subscription worker (i.e., apply worker and > > tablesync worker), not per relation. The subrelid is OID of the > > relation that the tablesync worker is synchronizing. For the stats of > > apply workers, it is null. > > That's precisely the misuse of the stats subsystem that I'm complaining about > here. The whole design of pgstat (as it is today) only makes sense if you can > loose a message and it doesn't matter much, because it's just an incremental > counter increment that's lost. And to be able properly prune dead pgstat > contents the underlying objects stats are kept around either need to be > permanent (e.g. stats about WAL) or a record of objects needs to exist > (e.g. stats about relations). > > > Even leaving everything else aside, a key of (dboid, subid, subrelid), where > subrelid can be NULL, but where (dboid, subid) is *not* unique, imo is poor > relational design. What is the justification for mixing relation specific and > non-relation specific contents in this view? I think the current schema of the view with key (dboid, subid, subrelid) comes from the fact that we store the same statistics for both apply and tablesync. I think even if we have two separate views for apply and tablesync, they would have almost the same columns except for their keys. Also, from the relational design point of view, pg_locks has a somewhat similar table schema; its database and relation columns can be NULL. > > > The patch you referenced [1] should just store the stats in the > pg_stat_subscription view, not pg_stat_subscription_workers. > > It *does* make sense to keep stats about the number of table syncs that failed > etc. But that should be a counter in pg_stat_subscription, not a row in > pg_stat_subscription_workers. We have discussed using pg_stat_subscription before but concluded it's not an appropriate place to store error information because it ends up keeping cumulative stats mixed with non-cumulative stats. To take a precedent, we used to store accumulative statistics such as spill_txns to pg_stat_replication, but then for the same reason we moved those statistics to the new stats view, pg_stat_replication_slot. New subscription statistics that we're introducing are cumulative statistics whereas pg_stat_subscription is a dynamic statistics view. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-21 05:10 Amit Kapila <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Amit Kapila @ 2022-02-21 05:10 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Andres Freund <[email protected]>; Masahiko Sawada <[email protected]>; pgsql-hackers On Sat, Feb 19, 2022 at 10:35 PM David G. Johnston <[email protected]> wrote: > > On Sat, Feb 19, 2022 at 9:37 AM Andres Freund <[email protected]> wrote: >> >> IMO the type of information you'd want for apply failures is substantially >> >> different enough from worker failures that I don't really see the temptation >> to put them in the same table. >> > > It's an error message and a transaction LSN in both cases right now, along with knowledge of whether said transaction only affects a single table (relid is not null) or not (relid is null). Do you have a concrete idea in mind that would make this separation need more obvious? > I would also like to mention that in some cases, sync workers also behaves like apply worker (after initial sync till it catches up with the apply worker) and try to stream and apply changes similar to apply worker. The error during that phase will be no different than the apply worker. One idea to make the separation more obvious is to introduce 'worker_type' column similar to backend_type in pg_stat_activity which will tell whether it is an apply worker or a table sync worker. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats @ 2022-02-21 16:19 David G. Johnston <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 0 replies; 23+ messages in thread From: David G. Johnston @ 2022-02-21 16:19 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Andres Freund <[email protected]>; Masahiko Sawada <[email protected]>; pgsql-hackers On Sun, Feb 20, 2022 at 10:10 PM Amit Kapila <[email protected]> wrote: > On Sat, Feb 19, 2022 at 10:35 PM David G. Johnston > <[email protected]> wrote: > > > > On Sat, Feb 19, 2022 at 9:37 AM Andres Freund <[email protected]> > wrote: > >> > >> IMO the type of information you'd want for apply failures is > substantially > >> > >> different enough from worker failures that I don't really see the > temptation > >> to put them in the same table. > >> > > > > It's an error message and a transaction LSN in both cases right now, > along with knowledge of whether said transaction only affects a single > table (relid is not null) or not (relid is null). Do you have a concrete > idea in mind that would make this separation need more obvious? > > > > I would also like to mention that in some cases, sync workers also > behaves like apply worker (after initial sync till it catches up with > the apply worker) and try to stream and apply changes similar to apply > worker. The error during that phase will be no different than the > apply worker. One idea to make the separation more obvious is to > introduce 'worker_type' column similar to backend_type in > pg_stat_activity which will tell whether it is an apply worker or a > table sync worker. > > The point isn't to make the separation more obvious by specifying which worker type is doing the work. It is to make the concept of worker type (and identity) irrelevant. The end user cannot (and should not be able to) address individual workers - only the subscription. Even while a sync worker is in synchronization mode (as opposed to whatever mode comes before synchronization mode) it still only affects a single table. To the end user the distinction between the two modes is immaterial. The statement "will be no different than the apply worker" doesn't make sense to me given that in a multiple-table subscription (the only kind where this matters...) you will have multiple table sync workers in synchronization mode and they both cannot behave identically to an apply worker otherwise they would step on each other's toes. That two different table-specific updates produce the same error shouldn't be a problem if that is indeed what happens (though if the error is on tableA having the worker for tableB report the tableA error would be odd - but not problematic). I'll admit I don't fully understand the details of this particular synchronization interaction but I'm not see how the discussion of "errors during table-specific updates" vs "errors during whole transaction application" can be affected by it. David J. ^ permalink raw reply [nested|flat] 23+ messages in thread
end of thread, other threads:[~2022-02-21 16:19 UTC | newest] Thread overview: 23+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-09-16 08:07 [PATCH v9 2/5] Move code around in StartupXLOG(). Heikki Linnakangas <[email protected]> 2022-02-03 04:33 Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]> 2022-02-03 04:48 ` Re: Design of pg_stat_subscription_workers vs pgstats David G. Johnston <[email protected]> 2022-02-15 18:26 ` Re: Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]> 2022-02-16 08:49 ` Re: Design of pg_stat_subscription_workers vs pgstats Amit Kapila <[email protected]> 2022-02-16 11:36 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]> 2022-02-18 08:26 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]> 2022-02-18 19:46 ` Re: Design of pg_stat_subscription_workers vs pgstats David G. Johnston <[email protected]> 2022-02-19 10:49 ` Re: Design of pg_stat_subscription_workers vs pgstats Amit Kapila <[email protected]> 2022-02-19 13:21 ` Re: Design of pg_stat_subscription_workers vs pgstats David G. Johnston <[email protected]> 2022-02-19 13:36 ` Re: Design of pg_stat_subscription_workers vs pgstats Amit Kapila <[email protected]> 2022-02-19 13:44 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]> 2022-02-19 13:43 ` Re: Design of pg_stat_subscription_workers vs pgstats Amit Kapila <[email protected]> 2022-02-19 13:48 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]> 2022-02-18 20:32 ` Re: Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]> 2022-02-19 13:38 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]> 2022-02-19 16:02 ` Re: Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]> 2022-02-19 16:16 ` Re: Design of pg_stat_subscription_workers vs pgstats David G. Johnston <[email protected]> 2022-02-19 16:37 ` Re: Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]> 2022-02-19 17:04 ` Re: Design of pg_stat_subscription_workers vs pgstats David G. Johnston <[email protected]> 2022-02-21 05:10 ` Re: Design of pg_stat_subscription_workers vs pgstats Amit Kapila <[email protected]> 2022-02-21 16:19 ` Re: Design of pg_stat_subscription_workers vs pgstats David G. Johnston <[email protected]> 2022-02-21 03:56 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox