public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v37 3/6] Make archiver process an auxiliary process 4+ messages / 3 participants [nested] [flat]
* [PATCH v37 3/6] Make archiver process an auxiliary process @ 2020-03-13 07:59 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Kyotaro Horiguchi @ 2020-03-13 07:59 UTC (permalink / raw) This is a preliminary patch for shared-memory based stats collector. Archiver process must be a auxiliary process since it uses shared memory after stats data was moved into shared-memory. Make the process an auxiliary process in order to make it work. --- src/backend/access/transam/xlogarchive.c | 6 +- src/backend/bootstrap/bootstrap.c | 22 ++-- src/backend/postmaster/pgarch.c | 130 +++-------------------- src/backend/postmaster/postmaster.c | 50 +++++---- src/backend/storage/lmgr/proc.c | 1 + src/include/access/xlog.h | 3 + src/include/access/xlogarchive.h | 1 + src/include/miscadmin.h | 2 + src/include/postmaster/pgarch.h | 4 +- src/include/storage/pmsignal.h | 1 - src/include/storage/proc.h | 3 + 11 files changed, 69 insertions(+), 154 deletions(-) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 8f8734dc1d..2e6c322142 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -29,7 +29,9 @@ #include "storage/fd.h" #include "storage/ipc.h" #include "storage/lwlock.h" +#include "storage/latch.h" #include "storage/pmsignal.h" +#include "storage/proc.h" /* * Attempt to retrieve the specified file from off-line archival storage. @@ -489,8 +491,8 @@ XLogArchiveNotify(const char *xlog) } /* Notify archiver that it's got something to do */ - if (IsUnderPostmaster) - SendPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER); + if (IsUnderPostmaster && ProcGlobal->archiverLatch) + SetLatch(ProcGlobal->archiverLatch); } /* diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 76b2f5066f..81bfaea869 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -317,6 +317,9 @@ AuxiliaryProcessMain(int argc, char *argv[]) case StartupProcess: MyBackendType = B_STARTUP; break; + case ArchiverProcess: + MyBackendType = B_ARCHIVER; + break; case BgWriterProcess: MyBackendType = B_BG_WRITER; break; @@ -437,30 +440,29 @@ AuxiliaryProcessMain(int argc, char *argv[]) proc_exit(1); /* should never return */ case StartupProcess: - /* don't set signals, startup process has its own agenda */ StartupProcessMain(); - proc_exit(1); /* should never return */ + proc_exit(1); + + case ArchiverProcess: + PgArchiverMain(); + proc_exit(1); case BgWriterProcess: - /* don't set signals, bgwriter has its own agenda */ BackgroundWriterMain(); - proc_exit(1); /* should never return */ + proc_exit(1); case CheckpointerProcess: - /* don't set signals, checkpointer has its own agenda */ CheckpointerMain(); - proc_exit(1); /* should never return */ + proc_exit(1); case WalWriterProcess: - /* don't set signals, walwriter has its own agenda */ InitXLOGAccess(); WalWriterMain(); - proc_exit(1); /* should never return */ + proc_exit(1); case WalReceiverProcess: - /* don't set signals, walreceiver has its own agenda */ WalReceiverMain(); - proc_exit(1); /* should never return */ + proc_exit(1); default: elog(PANIC, "unrecognized process type: %d", (int) MyAuxProcType); diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index ed1b65358d..e3a520def9 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -48,6 +48,7 @@ #include "storage/latch.h" #include "storage/pg_shmem.h" #include "storage/pmsignal.h" +#include "storage/procsignal.h" #include "utils/guc.h" #include "utils/ps_status.h" @@ -78,13 +79,11 @@ * Local data * ---------- */ -static time_t last_pgarch_start_time; static time_t last_sigterm_time = 0; /* * Flags set by interrupt handlers for later service in the main loop. */ -static volatile sig_atomic_t wakened = false; static volatile sig_atomic_t ready_to_stop = false; /* ---------- @@ -95,8 +94,6 @@ static volatile sig_atomic_t ready_to_stop = false; static pid_t pgarch_forkexec(void); #endif -NON_EXEC_STATIC void PgArchiverMain(int argc, char *argv[]) pg_attribute_noreturn(); -static void pgarch_waken(SIGNAL_ARGS); static void pgarch_waken_stop(SIGNAL_ARGS); static void pgarch_MainLoop(void); static void pgarch_ArchiverCopyLoop(void); @@ -110,75 +107,6 @@ static void pgarch_archiveDone(char *xlog); * ------------------------------------------------------------ */ -/* - * pgarch_start - * - * Called from postmaster at startup or after an existing archiver - * died. Attempt to fire up a fresh archiver process. - * - * Returns PID of child process, or 0 if fail. - * - * Note: if fail, we will be called again from the postmaster main loop. - */ -int -pgarch_start(void) -{ - time_t curtime; - pid_t pgArchPid; - - /* - * Do nothing if no archiver needed - */ - if (!XLogArchivingActive()) - return 0; - - /* - * Do nothing if too soon since last archiver start. This is a safety - * valve to protect against continuous respawn attempts if the archiver is - * dying immediately at launch. Note that since we will be re-called from - * the postmaster main loop, we will get another chance later. - */ - curtime = time(NULL); - if ((unsigned int) (curtime - last_pgarch_start_time) < - (unsigned int) PGARCH_RESTART_INTERVAL) - return 0; - last_pgarch_start_time = curtime; - -#ifdef EXEC_BACKEND - switch ((pgArchPid = pgarch_forkexec())) -#else - switch ((pgArchPid = fork_process())) -#endif - { - case -1: - ereport(LOG, - (errmsg("could not fork archiver: %m"))); - return 0; - -#ifndef EXEC_BACKEND - case 0: - /* in postmaster child ... */ - InitPostmasterChild(); - - /* Close the postmaster's sockets */ - ClosePostmasterPorts(false); - - /* Drop our connection to postmaster's shared memory, as well */ - dsm_detach_all(); - PGSharedMemoryDetach(); - - PgArchiverMain(0, NULL); - break; -#endif - - default: - return (int) pgArchPid; - } - - /* shouldn't get here */ - return 0; -} - /* ------------------------------------------------------------ * Local functions called by archiver follow * ------------------------------------------------------------ @@ -212,14 +140,9 @@ pgarch_forkexec(void) #endif /* EXEC_BACKEND */ -/* - * PgArchiverMain - * - * The argc/argv parameters are valid only in EXEC_BACKEND case. However, - * since we don't use 'em, it hardly matters... - */ -NON_EXEC_STATIC void -PgArchiverMain(int argc, char *argv[]) +/* Main entry point for archiver process */ +void +PgArchiverMain(void) { /* * Ignore all signals usually bound to some action in the postmaster, @@ -231,33 +154,26 @@ PgArchiverMain(int argc, char *argv[]) /* SIGQUIT handler was already set up by InitPostmasterChild */ pqsignal(SIGALRM, SIG_IGN); pqsignal(SIGPIPE, SIG_IGN); - pqsignal(SIGUSR1, pgarch_waken); + pqsignal(SIGUSR1, procsignal_sigusr1_handler); pqsignal(SIGUSR2, pgarch_waken_stop); + /* Reset some signals that are accepted by postmaster but not here */ pqsignal(SIGCHLD, SIG_DFL); + + /* Unblock signals (they were blocked when the postmaster forked us) */ PG_SETMASK(&UnBlockSig); - MyBackendType = B_ARCHIVER; - init_ps_display(NULL); + /* + * Advertise our latch that backends can use to wake us up while we're + * sleeping. + */ + ProcGlobal->archiverLatch = &MyProc->procLatch; pgarch_MainLoop(); exit(0); } -/* SIGUSR1 signal handler for archiver process */ -static void -pgarch_waken(SIGNAL_ARGS) -{ - int save_errno = errno; - - /* set flag that there is work to be done */ - wakened = true; - SetLatch(MyLatch); - - errno = save_errno; -} - /* SIGUSR2 signal handler for archiver process */ static void pgarch_waken_stop(SIGNAL_ARGS) @@ -282,14 +198,6 @@ pgarch_MainLoop(void) pg_time_t last_copy_time = 0; bool time_to_stop; - /* - * We run the copy loop immediately upon entry, in case there are - * unarchived files left over from a previous database run (or maybe the - * archiver died unexpectedly). After that we wait for a signal or - * timeout before doing more. - */ - wakened = true; - /* * There shouldn't be anything for the archiver to do except to wait for a * signal ... however, the archiver exists to protect our data, so she @@ -328,12 +236,8 @@ pgarch_MainLoop(void) } /* Do what we're here for */ - if (wakened || time_to_stop) - { - wakened = false; - pgarch_ArchiverCopyLoop(); - last_copy_time = time(NULL); - } + pgarch_ArchiverCopyLoop(); + last_copy_time = time(NULL); /* * Sleep until a signal is received, or until a poll is forced by @@ -354,13 +258,9 @@ pgarch_MainLoop(void) WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH, timeout * 1000L, WAIT_EVENT_ARCHIVER_MAIN); - if (rc & WL_TIMEOUT) - wakened = true; if (rc & WL_POSTMASTER_DEATH) time_to_stop = true; } - else - wakened = true; } /* diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 959e3b8873..b811c961a6 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -555,6 +555,7 @@ static void ShmemBackendArrayRemove(Backend *bn); #endif /* EXEC_BACKEND */ #define StartupDataBase() StartChildProcess(StartupProcess) +#define StartArchiver() StartChildProcess(ArchiverProcess) #define StartBackgroundWriter() StartChildProcess(BgWriterProcess) #define StartCheckpointer() StartChildProcess(CheckpointerProcess) #define StartWalWriter() StartChildProcess(WalWriterProcess) @@ -1800,7 +1801,7 @@ ServerLoop(void) /* If we have lost the archiver, try to start a new one. */ if (PgArchPID == 0 && PgArchStartupAllowed()) - PgArchPID = pgarch_start(); + PgArchPID = StartArchiver(); /* If we need to signal the autovacuum launcher, do so now */ if (avlauncher_needs_signal) @@ -3054,7 +3055,7 @@ reaper(SIGNAL_ARGS) if (!IsBinaryUpgrade && AutoVacuumingActive() && AutoVacPID == 0) AutoVacPID = StartAutoVacLauncher(); if (PgArchStartupAllowed() && PgArchPID == 0) - PgArchPID = pgarch_start(); + PgArchPID = StartArchiver(); if (PgStatPID == 0) PgStatPID = pgstat_start(); @@ -3189,20 +3190,16 @@ reaper(SIGNAL_ARGS) } /* - * Was it the archiver? If so, just try to start a new one; no need - * to force reset of the rest of the system. (If fail, we'll try - * again in future cycles of the main loop.). Unless we were waiting - * for it to shut down; don't restart it in that case, and - * PostmasterStateMachine() will advance to the next shutdown step. + * Was it the archiver? Normal exit can be ignored; we'll start a new + * one at the next iteration of the postmaster's main loop, if + * necessary. Any other exit condition is treated as a crash. */ if (pid == PgArchPID) { PgArchPID = 0; if (!EXIT_STATUS_0(exitstatus)) - LogChildExit(LOG, _("archiver process"), - pid, exitstatus); - if (PgArchStartupAllowed()) - PgArchPID = pgarch_start(); + HandleChildCrash(pid, exitstatus, + _("archiver process")); continue; } @@ -3450,7 +3447,7 @@ CleanupBackend(int pid, /* * HandleChildCrash -- cleanup after failed backend, bgwriter, checkpointer, - * walwriter, autovacuum, or background worker. + * walwriter, autovacuum, archiver or background worker. * * The objectives here are to clean up our local state about the child * process, and to signal all other remaining children to quickdie. @@ -3655,6 +3652,18 @@ HandleChildCrash(int pid, int exitstatus, const char *procname) signal_child(AutoVacPID, (SendStop ? SIGSTOP : SIGQUIT)); } + /* Take care of the archiver too */ + if (pid == PgArchPID) + PgArchPID = 0; + else if (PgArchPID != 0 && take_action) + { + ereport(DEBUG2, + (errmsg_internal("sending %s to process %d", + (SendStop ? "SIGSTOP" : "SIGQUIT"), + (int) PgArchPID))); + signal_child(PgArchPID, (SendStop ? SIGSTOP : SIGQUIT)); + } + /* * Force a power-cycle of the pgarch process too. (This isn't absolutely * necessary, but it seems like a good idea for robustness, and it @@ -3951,6 +3960,7 @@ PostmasterStateMachine(void) Assert(CheckpointerPID == 0); Assert(WalWriterPID == 0); Assert(AutoVacPID == 0); + Assert(PgArchPID == 0); /* syslogger is not considered here */ pmState = PM_NO_CHILDREN; } @@ -5230,7 +5240,7 @@ sigusr1_handler(SIGNAL_ARGS) */ Assert(PgArchPID == 0); if (XLogArchivingAlways()) - PgArchPID = pgarch_start(); + PgArchPID = StartArchiver(); /* * If we aren't planning to enter hot standby mode later, treat @@ -5275,16 +5285,6 @@ sigusr1_handler(SIGNAL_ARGS) if (StartWorkerNeeded || HaveCrashedWorker) maybe_start_bgworkers(); - if (CheckPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER) && - PgArchPID != 0) - { - /* - * Send SIGUSR1 to archiver process, to wake it up and begin archiving - * next WAL file. - */ - signal_child(PgArchPID, SIGUSR1); - } - /* Tell syslogger to rotate logfile if requested */ if (SysLoggerPID != 0) { @@ -5526,6 +5526,10 @@ StartChildProcess(AuxProcType type) ereport(LOG, (errmsg("could not fork startup process: %m"))); break; + case ArchiverProcess: + ereport(LOG, + (errmsg("could not fork archiver process: %m"))); + break; case BgWriterProcess: ereport(LOG, (errmsg("could not fork background writer process: %m"))); diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 88566bd9fa..746bed773e 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -181,6 +181,7 @@ InitProcGlobal(void) ProcGlobal->startupBufferPinWaitBufId = -1; ProcGlobal->walwriterLatch = NULL; ProcGlobal->checkpointerLatch = NULL; + ProcGlobal->archiverLatch = NULL; pg_atomic_init_u32(&ProcGlobal->procArrayGroupFirst, INVALID_PGPROCNO); pg_atomic_init_u32(&ProcGlobal->clogGroupFirst, INVALID_PGPROCNO); diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 221af87e71..65443063e8 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -339,6 +339,9 @@ extern XLogRecPtr GetRedoRecPtr(void); extern XLogRecPtr GetInsertRecPtr(void); extern XLogRecPtr GetFlushRecPtr(void); extern XLogRecPtr GetLastImportantRecPtr(void); +extern void XLogArchiveWakeupStart(void); +extern void XLogArchiveWakeupEnd(void); +extern void XLogArchiveWakeup(void); extern void RemovePromoteSignalFiles(void); extern bool PromoteIsTriggered(void); diff --git a/src/include/access/xlogarchive.h b/src/include/access/xlogarchive.h index 1c67de2ede..54ce0b97d7 100644 --- a/src/include/access/xlogarchive.h +++ b/src/include/access/xlogarchive.h @@ -25,6 +25,7 @@ extern void ExecuteRecoveryCommand(const char *command, const char *commandName, extern void KeepFileRestoredFromArchive(const char *path, const char *xlogfname); extern void XLogArchiveNotify(const char *xlog); extern void XLogArchiveNotifySeg(XLogSegNo segno); +extern void XLogArchiveWakeup(void); extern void XLogArchiveForceDone(const char *xlog); extern bool XLogArchiveCheckDone(const char *xlog); extern bool XLogArchiveIsBusy(const char *xlog); diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 72e3352398..bba002ad24 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -418,6 +418,7 @@ typedef enum BootstrapProcess, StartupProcess, BgWriterProcess, + ArchiverProcess, CheckpointerProcess, WalWriterProcess, WalReceiverProcess, @@ -430,6 +431,7 @@ extern AuxProcType MyAuxProcType; #define AmBootstrapProcess() (MyAuxProcType == BootstrapProcess) #define AmStartupProcess() (MyAuxProcType == StartupProcess) #define AmBackgroundWriterProcess() (MyAuxProcType == BgWriterProcess) +#define AmArchiverProcess() (MyAuxProcType == ArchiverProcess) #define AmCheckpointerProcess() (MyAuxProcType == CheckpointerProcess) #define AmWalWriterProcess() (MyAuxProcType == WalWriterProcess) #define AmWalReceiverProcess() (MyAuxProcType == WalReceiverProcess) diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h index b3200874ca..e3ffc63f14 100644 --- a/src/include/postmaster/pgarch.h +++ b/src/include/postmaster/pgarch.h @@ -32,8 +32,6 @@ */ extern int pgarch_start(void); -#ifdef EXEC_BACKEND -extern void PgArchiverMain(int argc, char *argv[]) pg_attribute_noreturn(); -#endif +extern void PgArchiverMain(void) pg_attribute_noreturn(); #endif /* _PGARCH_H */ diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h index 56c5ec4481..c691acf8cd 100644 --- a/src/include/storage/pmsignal.h +++ b/src/include/storage/pmsignal.h @@ -34,7 +34,6 @@ typedef enum { PMSIGNAL_RECOVERY_STARTED, /* recovery has started */ PMSIGNAL_BEGIN_HOT_STANDBY, /* begin Hot Standby */ - PMSIGNAL_WAKEN_ARCHIVER, /* send a NOTIFY signal to xlog archiver */ PMSIGNAL_ROTATE_LOGFILE, /* send SIGUSR1 to syslogger to rotate logfile */ PMSIGNAL_START_AUTOVAC_LAUNCHER, /* start an autovacuum launcher */ PMSIGNAL_START_AUTOVAC_WORKER, /* start an autovacuum worker */ diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 9c9a50ae45..de20520b8c 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -345,6 +345,9 @@ typedef struct PROC_HDR int startupProcPid; /* Buffer id of the buffer that Startup process waits for pin on, or -1 */ int startupBufferPinWaitBufId; + /* Archiver process's latch */ + Latch *archiverLatch; + /* Current shared estimate of appropriate spins_per_delay value */ } PROC_HDR; extern PGDLLIMPORT PROC_HDR *ProcGlobal; -- 2.18.4 ----Next_Part(Fri_Sep_25_09_27_26_2020_419)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v37-0004-Shared-memory-based-stats-collector.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Row pattern recognition @ 2024-09-28 10:43 Tatsuo Ishii <[email protected]> 2024-09-30 00:07 ` Re: Row pattern recognition Tatsuo Ishii <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Tatsuo Ishii @ 2024-09-28 10:43 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; pgsql-hackers; [email protected]; [email protected] > With some bigger partitions, I hit an `ERROR: wrong pos: 1024`. A > test that reproduces it is attached. Thanks for the report. Attached is a patch on top of v22 patches to fix the bug. We keep info in an array (WindowAggState.reduced_frame_map) to track the rpr pattern match result status for each row in a frame. If pattern match succeeds, the first row in the reduced frame has status RF_FRAME_HEAD and rest of rows have RF_SKIPPED state. A row with pattern match failure state has RF_UNMATCHED state. Any row which is never tested has state RF_NOT_DETERMINED. At begining the map is initialized with 1024 entries with all RF_NOT_DETERMINED state. Eventually they are replaced with other than RF_NOT_DETERMINED state. In the error case rpr engine tries to find 1024 th row's state in the map and failed because the row's state has not been tested yet. I think we should treat it as RF_NOT_DETERMINED rather than an error. Attached patch does it. > While playing with the feature, I've been trying to identify runs of > matched rows by eye. But it's pretty difficult -- the best I can do is > manually count rows using a `COUNT(*) OVER ...`. So I'd like to > suggest that MEASURES be part of the eventual v1 feature, if there's > no other way to determine whether a row was skipped by a previous > match. (That was less obvious to me before the fix in v17.) I think implementing MEASURES is challenging. Especially we need to find how our parser accepts "colname OVER window_definition". Currently PostgreSQL's parser only accepts "func() OVER window_definition" Even it is technically possible, I think the v1 patch size will become much larger than now due to this. How about inventing new window function that returns row state instead? - match found (yes/no) - skipped due to AFTER MATCH SKIP PAST LAST ROW (no match) For the rest of the mail I need more time to understand. I will reply back after studying it. For now, I just want to thank you for the valuable information! > -- > > I've been working on an implementation [1] of SQL/RPR's "parenthesized > language" and preferment order. (These are defined in SQL/Foundation > 2023, section 9.41.) The tool gives you a way to figure out, for a > given pattern, what matches are supposed to be attempted and in what > order: > > $ ./src/test/modules/rpr/rpr_prefer "a b? a" > ( ( a ( b ) ) a ) > ( ( a ( ) ) a ) > > Many simple patterns result in an infinite set of possible matches. So > if you use an unbounded quantifiers, you have to also use --max-rows > to limit the size of the hypothetical window frame: > > $ ./src/test/modules/rpr/rpr_prefer --max-rows 2 "^ PERMUTE(a*, b+)? $" > ( ( ^ ( ( ( ( ( ( a ) ( b ) ) ) - ) ) ) ) $ ) > ( ( ^ ( ( ( ( ( ( ) ( b b ) ) ) - ) ) ) ) $ ) > ( ( ^ ( ( ( ( ( ( ) ( b ) ) ) - ) ) ) ) $ ) > ( ( ^ ( ( ( - ( ( ( b b ) ( ) ) ) ) ) ) ) $ ) > ( ( ^ ( ( ( - ( ( ( b ) ( a ) ) ) ) ) ) ) $ ) > ( ( ^ ( ( ( - ( ( ( b ) ( ) ) ) ) ) ) ) $ ) > ( ( ^ ( ) ) $ ) > > I've found this useful to check my personal understanding of the spec > and the match behavior, but it could also potentially be used to > generate test cases, or to help users debug their own patterns. For > example, a pattern that has a bunch of duplicate sequences in its PL > is probably not very well optimized: > > $ ./src/test/modules/rpr/rpr_prefer --max-rows 4 "a+ a+" > ( ( a a a ) ( a ) ) > ( ( a a ) ( a a ) ) > ( ( a a ) ( a ) ) > ( ( a ) ( a a a ) ) > ( ( a ) ( a a ) ) > ( ( a ) ( a ) ) > > And patterns with catastrophic backtracking behavior tend to show a > "sawtooth" pattern in the output, with a huge number of potential > matches being generated relative to the number of rows in the frame. > > My implementation is really messy -- it leaks memory like a sieve, and > I cannibalized the parser from ECPG, which just ended up as an > exercise in teaching myself flex/bison. But if there's interest in > having this kind of tool in the tree, I can work on making it > reviewable. Either way, I should be able to use it to double-check > more complicated test cases. > > A while back [2], you were wondering whether our Bison implementation > would be able to parse the PATTERN grammar directly. I think this tool > proves that the answer is "yes", but PERMUTE in particular causes a > shift/reduce conflict. To fix it, I applied the same precedence > workaround that we use for CUBE and ROLLUP. > > Thanks again! > --Jacob > > [1] https://github.com/jchampio/postgres/tree/dev/rpr > [2] https://www.postgresql.org/message-id/20230721.151648.412762379013769790.t-ishii%40sranhm.sra.co.jp diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index e46a3dd1b7..4a5a6fbf07 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -4148,9 +4148,15 @@ int get_reduced_frame_map(WindowAggState *winstate, int64 pos) { Assert(winstate->reduced_frame_map != NULL); + Assert(pos >= 0); - if (pos < 0 || pos >= winstate->alloc_sz) - elog(ERROR, "wrong pos: " INT64_FORMAT, pos); + /* + * If pos is not in the reduced frame map, it means that any info + * regarding the pos has not been registered yet. So we return + * RF_NOT_DETERMINED. + */ + if (pos >= winstate->alloc_sz) + return RF_NOT_DETERMINED; return winstate->reduced_frame_map[pos]; } Attachments: [text/plain] rpr_fix.txt (741B, ../../[email protected]/2-rpr_fix.txt) download | inline diff: diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index e46a3dd1b7..4a5a6fbf07 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -4148,9 +4148,15 @@ int get_reduced_frame_map(WindowAggState *winstate, int64 pos) { Assert(winstate->reduced_frame_map != NULL); + Assert(pos >= 0); - if (pos < 0 || pos >= winstate->alloc_sz) - elog(ERROR, "wrong pos: " INT64_FORMAT, pos); + /* + * If pos is not in the reduced frame map, it means that any info + * regarding the pos has not been registered yet. So we return + * RF_NOT_DETERMINED. + */ + if (pos >= winstate->alloc_sz) + return RF_NOT_DETERMINED; return winstate->reduced_frame_map[pos]; } ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Row pattern recognition 2024-09-28 10:43 Re: Row pattern recognition Tatsuo Ishii <[email protected]> @ 2024-09-30 00:07 ` Tatsuo Ishii <[email protected]> 2024-10-01 12:48 ` Re: Row pattern recognition Jacob Champion <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Tatsuo Ishii @ 2024-09-30 00:07 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; pgsql-hackers; [email protected]; [email protected] >> While playing with the feature, I've been trying to identify runs of >> matched rows by eye. But it's pretty difficult -- the best I can do is >> manually count rows using a `COUNT(*) OVER ...`. So I'd like to >> suggest that MEASURES be part of the eventual v1 feature, if there's >> no other way to determine whether a row was skipped by a previous >> match. (That was less obvious to me before the fix in v17.) > > I think implementing MEASURES is challenging. Especially we need to > find how our parser accepts "colname OVER > window_definition". Currently PostgreSQL's parser only accepts "func() > OVER window_definition" Even it is technically possible, I think the > v1 patch size will become much larger than now due to this. > > How about inventing new window function that returns row state instead? > > - match found (yes/no) > - skipped due to AFTER MATCH SKIP PAST LAST ROW (no match) Please disregard my proposal. Even if we make such a function, it would always return NULL for unmatched rows or skipped rows, and I think the function does not solve your problem. However, I wonder if supporting MEASURES solves the problem either because any columns defined by MEASURES will return NULL except the first row in a reduced frame. Can you please show an example how to identify runs of matched rows using MEASURES? > For the rest of the mail I need more time to understand. I will reply > back after studying it. For now, I just want to thank you for the > valuable information! > >> -- >> >> I've been working on an implementation [1] of SQL/RPR's "parenthesized >> language" and preferment order. (These are defined in SQL/Foundation >> 2023, section 9.41.) The tool gives you a way to figure out, for a >> given pattern, what matches are supposed to be attempted and in what >> order: >> >> $ ./src/test/modules/rpr/rpr_prefer "a b? a" >> ( ( a ( b ) ) a ) >> ( ( a ( ) ) a ) >> >> Many simple patterns result in an infinite set of possible matches. So >> if you use an unbounded quantifiers, you have to also use --max-rows >> to limit the size of the hypothetical window frame: >> >> $ ./src/test/modules/rpr/rpr_prefer --max-rows 2 "^ PERMUTE(a*, b+)? $" >> ( ( ^ ( ( ( ( ( ( a ) ( b ) ) ) - ) ) ) ) $ ) >> ( ( ^ ( ( ( ( ( ( ) ( b b ) ) ) - ) ) ) ) $ ) >> ( ( ^ ( ( ( ( ( ( ) ( b ) ) ) - ) ) ) ) $ ) >> ( ( ^ ( ( ( - ( ( ( b b ) ( ) ) ) ) ) ) ) $ ) >> ( ( ^ ( ( ( - ( ( ( b ) ( a ) ) ) ) ) ) ) $ ) >> ( ( ^ ( ( ( - ( ( ( b ) ( ) ) ) ) ) ) ) $ ) >> ( ( ^ ( ) ) $ ) I wonder how Oracle solves the problem (an infinite set of possible matches) without using "--max-rows" or something like that because in my understanding Oracle supports the regular expressions and PERMUTE. >> I've found this useful to check my personal understanding of the spec >> and the match behavior, but it could also potentially be used to >> generate test cases, or to help users debug their own patterns. For >> example, a pattern that has a bunch of duplicate sequences in its PL >> is probably not very well optimized: >> >> $ ./src/test/modules/rpr/rpr_prefer --max-rows 4 "a+ a+" >> ( ( a a a ) ( a ) ) >> ( ( a a ) ( a a ) ) >> ( ( a a ) ( a ) ) >> ( ( a ) ( a a a ) ) >> ( ( a ) ( a a ) ) >> ( ( a ) ( a ) ) >> >> And patterns with catastrophic backtracking behavior tend to show a >> "sawtooth" pattern in the output, with a huge number of potential >> matches being generated relative to the number of rows in the frame. >> >> My implementation is really messy -- it leaks memory like a sieve, and >> I cannibalized the parser from ECPG, which just ended up as an >> exercise in teaching myself flex/bison. But if there's interest in >> having this kind of tool in the tree, I can work on making it >> reviewable. Either way, I should be able to use it to double-check >> more complicated test cases. I definitely am interested in the tool! >> A while back [2], you were wondering whether our Bison implementation >> would be able to parse the PATTERN grammar directly. I think this tool >> proves that the answer is "yes", but PERMUTE in particular causes a >> shift/reduce conflict. To fix it, I applied the same precedence >> workaround that we use for CUBE and ROLLUP. That's a good news! Best reagards, -- Tatsuo Ishii SRA OSS K.K. English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Row pattern recognition 2024-09-28 10:43 Re: Row pattern recognition Tatsuo Ishii <[email protected]> 2024-09-30 00:07 ` Re: Row pattern recognition Tatsuo Ishii <[email protected]> @ 2024-10-01 12:48 ` Jacob Champion <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Jacob Champion @ 2024-10-01 12:48 UTC (permalink / raw) To: Tatsuo Ishii <[email protected]>; +Cc: [email protected]; pgsql-hackers; [email protected]; [email protected] On Sun, Sep 29, 2024 at 5:08 PM Tatsuo Ishii <[email protected]> wrote: > > I think implementing MEASURES is challenging. Especially we need to > > find how our parser accepts "colname OVER > > window_definition". Currently PostgreSQL's parser only accepts "func() > > OVER window_definition" Even it is technically possible, I think the > > v1 patch size will become much larger than now due to this. [resending, to the whole list this time] Yeah. In any case, I'm not the right person to bolt MEASURES onto the existing grammar... my misadventures in the PATTERN parser have highlighted how little I know about Bison. :D > Please disregard my proposal. Even if we make such a function, it > would always return NULL for unmatched rows or skipped rows, and I > think the function does not solve your problem. > > However, I wonder if supporting MEASURES solves the problem either > because any columns defined by MEASURES will return NULL except the > first row in a reduced frame. Can you please show an example how to > identify runs of matched rows using MEASURES? I think you're probably right; my suggestion can't distinguish between skipped (but previously matched) rows and entirely-unmatched rows. The test case I'd been working with returned an empty match as a fallback, so it wouldn't have had that problem in practice. I was hoping that one of the existing whole-partition window functions would allow me to cobble something together based on the COUNT(*) measure, but after searching for a while I haven't been able to come up with a solution. Maybe it's just too niche for the window-function version of this -- after all, it only makes sense when using both INITIAL and AFTER MATCH SKIP PAST LAST ROW. A more general solution could identify the row_number of the first and last rows of the window frame, perhaps? But a frame isn't guaranteed to be contiguous, so maybe that doesn't make sense either. Ugh. > I wonder how Oracle solves the problem (an infinite set of possible > matches) without using "--max-rows" or something like that because in > my understanding Oracle supports the regular expressions and PERMUTE. I chose a confusing way to describe it, sorry. The parenthesized language for a pattern can be an infinite set, because A+ could match "( A )" or "( A A )" or "( A A A )" and so on forever. But that doesn't apply to our regex engine in practice; our tables have a finite number of rows, and I *think* the PL for a finite number of rows is also finite, due to the complicated rules on where empty matches are allowed to appear in the language. (In any case, my tool doesn't guard against infinite recursion...) > >> My implementation is really messy -- it leaks memory like a sieve, and > >> I cannibalized the parser from ECPG, which just ended up as an > >> exercise in teaching myself flex/bison. But if there's interest in > >> having this kind of tool in the tree, I can work on making it > >> reviewable. Either way, I should be able to use it to double-check > >> more complicated test cases. > > I definitely am interested in the tool! Okay, good to know! I will need to clean it up considerably, and figure out whether I've duplicated more code than I should have. > >> A while back [2], you were wondering whether our Bison implementation > >> would be able to parse the PATTERN grammar directly. I think this tool > >> proves that the answer is "yes", but PERMUTE in particular causes a > >> shift/reduce conflict. To fix it, I applied the same precedence > >> workaround that we use for CUBE and ROLLUP. > > That's a good news! +1 Thanks, --Jacob ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-10-01 12:48 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-13 07:59 [PATCH v37 3/6] Make archiver process an auxiliary process Kyotaro Horiguchi <[email protected]> 2024-09-28 10:43 Re: Row pattern recognition Tatsuo Ishii <[email protected]> 2024-09-30 00:07 ` Re: Row pattern recognition Tatsuo Ishii <[email protected]> 2024-10-01 12:48 ` Re: Row pattern recognition Jacob Champion <[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