public inbox for [email protected]help / color / mirror / Atom feed
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set 11+ messages / 4 participants [nested] [flat]
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2023-09-20 12:14 Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Daniel Gustafsson @ 2023-09-20 12:14 UTC (permalink / raw) To: bt23nguyent <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] > On 19 Sep 2023, at 11:21, bt23nguyent <[email protected]> wrote: > Please let me know if you have any further suggestions that I can improve more. + *logdetail = pstrdup("WAL archiving failed because basic_archive.archive_directory is not set"); Nitpick: detail messages should end with a period per the error message style guide [0]. - archiving will proceed only when it returns <literal>true</literal>. + archiving will proceed only when it returns <literal>true</literal>. The + archiver may also emit the detail explaining how the module is not configured + to the sever log if the archive module has any. I think this paragraph needs to be updated to include how the returned logdetail is emitted, since it currently shows the WARNING without mentioning the added detail in case returned. It would also be good to mention that it should be an allocated string which the caller can free. -- Daniel Gustafsson [0] https://www.postgresql.org/docs/devel/error-style-guide.html ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2023-09-21 02:18 bt23nguyent <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: bt23nguyent @ 2023-09-21 02:18 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] On 2023-09-20 21:14, Daniel Gustafsson wrote: >> On 19 Sep 2023, at 11:21, bt23nguyent <[email protected]> >> wrote: > >> Please let me know if you have any further suggestions that I can >> improve more. > > + *logdetail = pstrdup("WAL archiving failed because > basic_archive.archive_directory is not set"); > > Nitpick: detail messages should end with a period per the error message > style > guide [0]. > Yes! I totally missed this detail. > - archiving will proceed only when it returns > <literal>true</literal>. > + archiving will proceed only when it returns > <literal>true</literal>. The > + archiver may also emit the detail explaining how the module is > not configured > + to the sever log if the archive module has any. > > I think this paragraph needs to be updated to include how the returned > logdetail is emitted, since it currently shows the WARNING without > mentioning > the added detail in case returned. It would also be good to mention > that it > should be an allocated string which the caller can free. > > -- > Daniel Gustafsson > > [0] https://www.postgresql.org/docs/devel/error-style-guide.html Thank you for your kind review comment! I agree with you that this document update is not explanatory enough. So here is an updated patch. If there is any further suggestion, please let me know. Best regards, Tung Nguyen Attachments: [text/x-diff] v3-0001-Improve-log-message-output-of-basic-archive.patch (5.1K, ../../[email protected]/2-v3-0001-Improve-log-message-output-of-basic-archive.patch) download | inline diff: diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c index 4d78c31859..dd0f2816dc 100644 --- a/contrib/basic_archive/basic_archive.c +++ b/contrib/basic_archive/basic_archive.c @@ -48,7 +48,7 @@ typedef struct BasicArchiveData static char *archive_directory = NULL; static void basic_archive_startup(ArchiveModuleState *state); -static bool basic_archive_configured(ArchiveModuleState *state); +static bool basic_archive_configured(ArchiveModuleState *state, char **logdetail); static bool basic_archive_file(ArchiveModuleState *state, const char *file, const char *path); static void basic_archive_file_internal(const char *file, const char *path); static bool check_archive_directory(char **newval, void **extra, GucSource source); @@ -159,9 +159,15 @@ check_archive_directory(char **newval, void **extra, GucSource source) * Checks that archive_directory is not blank. */ static bool -basic_archive_configured(ArchiveModuleState *state) +basic_archive_configured(ArchiveModuleState *state, char **logdetail) { - return archive_directory != NULL && archive_directory[0] != '\0'; + if (archive_directory == NULL || archive_directory[0] == '\0') + { + *logdetail = pstrdup("WAL archiving failed because basic_archive.archive_directory is not set."); + return false; + } + else + return true; } /* diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml index 7064307d9e..b58ed238b4 100644 --- a/doc/src/sgml/archive-modules.sgml +++ b/doc/src/sgml/archive-modules.sgml @@ -101,7 +101,7 @@ typedef void (*ArchiveStartupCB) (ArchiveModuleState *state); assumes the module is configured. <programlisting> -typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state); +typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state, char **logdetail); </programlisting> If <literal>true</literal> is returned, the server will proceed with @@ -112,7 +112,10 @@ typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state); WARNING: archive_mode enabled, yet archiving is not configured </screen> In the latter case, the server will periodically call this function, and - archiving will proceed only when it returns <literal>true</literal>. + archiving will proceed only when it returns <literal>true</literal>. The + archiver may also emit the detail explaining how the module is not configured + to the sever log if the archive module has any. The detail message of archive + module should be an allocated string which the caller can free. </para> </sect2> diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c index 157ca4c751..c957a18ee7 100644 --- a/src/backend/archive/shell_archive.c +++ b/src/backend/archive/shell_archive.c @@ -23,7 +23,7 @@ #include "common/percentrepl.h" #include "pgstat.h" -static bool shell_archive_configured(ArchiveModuleState *state); +static bool shell_archive_configured(ArchiveModuleState *state, char **logdetail); static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path); @@ -43,7 +43,7 @@ shell_archive_init(void) } static bool -shell_archive_configured(ArchiveModuleState *state) +shell_archive_configured(ArchiveModuleState *state, char **logdetail) { return XLogArchiveCommand[0] != '\0'; } diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 46af349564..2fd1d03b09 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -390,7 +390,8 @@ pgarch_ArchiverCopyLoop(void) { struct stat stat_buf; char pathname[MAXPGPATH]; - + char *logdetail; + /* * Do not initiate any more archive commands after receiving * SIGTERM, nor after the postmaster has died unexpectedly. The @@ -410,10 +411,13 @@ pgarch_ArchiverCopyLoop(void) /* can't do anything if not configured ... */ if (ArchiveCallbacks->check_configured_cb != NULL && - !ArchiveCallbacks->check_configured_cb(archive_module_state)) + !ArchiveCallbacks->check_configured_cb(archive_module_state, &logdetail)) { ereport(WARNING, - (errmsg("archive_mode enabled, yet archiving is not configured"))); + (errmsg("archive_mode enabled, yet archiving is not configured")), + (logdetail != NULL) ? errdetail("%s", logdetail) : 0); + if (logdetail != NULL) + pfree(logdetail); return; } diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h index 679ce5a6db..43bd0ff4c2 100644 --- a/src/include/archive/archive_module.h +++ b/src/include/archive/archive_module.h @@ -36,7 +36,7 @@ typedef struct ArchiveModuleState * archive modules documentation. */ typedef void (*ArchiveStartupCB) (ArchiveModuleState *state); -typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state); +typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state, char **logdetail); typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path); typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state); ^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v7 2/7] Row pattern recognition patch (parse/analysis). @ 2023-09-22 04:53 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Tatsuo Ishii @ 2023-09-22 04:53 UTC (permalink / raw) --- src/backend/parser/parse_agg.c | 7 + src/backend/parser/parse_clause.c | 293 +++++++++++++++++++++++++++++- src/backend/parser/parse_expr.c | 4 + src/backend/parser/parse_func.c | 3 + 4 files changed, 306 insertions(+), 1 deletion(-) diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index 85cd47b7ae..aa7a1cee80 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -564,6 +564,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr) errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; + /* * There is intentionally no default: case here, so that the * compiler will warn if we add a new ParseExprKind without @@ -953,6 +957,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc, case EXPR_KIND_CYCLE_MARK: errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index 334b9b42bd..293d4b1680 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -100,7 +100,10 @@ static WindowClause *findWindowClause(List *wclist, const char *name); static Node *transformFrameOffset(ParseState *pstate, int frameOptions, Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc, Node *clause); - +static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist); +static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist); +static void transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef); +static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef); /* * transformFromClause - @@ -2950,6 +2953,10 @@ transformWindowDefinitions(ParseState *pstate, rangeopfamily, rangeopcintype, &wc->endInRangeFunc, windef->endOffset); + + /* Process Row Pattern Recognition related clauses */ + transformRPR(pstate, wc, windef, targetlist); + wc->runCondition = NIL; wc->winref = winref; @@ -3815,3 +3822,287 @@ transformFrameOffset(ParseState *pstate, int frameOptions, return node; } + +/* + * transformRPR + * Process Row Pattern Recognition related clauses + */ +static void +transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist) +{ + /* + * Window definition exists? + */ + if (windef == NULL) + return; + + /* + * Row Pattern Common Syntax clause exists? + */ + if (windef->rpCommonSyntax == NULL) + return; + + /* Check Frame option. Frame must start at current row */ + if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("FRAME must start at current row when row patttern recognition is used"))); + + /* Transform AFTER MACH SKIP TO clause */ + wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo; + + /* Transform SEEK or INITIAL clause */ + wc->initial = windef->rpCommonSyntax->initial; + + /* Transform DEFINE clause into list of TargetEntry's */ + wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist); + + /* Check PATTERN clause and copy to patternClause */ + transformPatternClause(pstate, wc, windef); + + /* Transform MEASURE clause */ + transformMeasureClause(pstate, wc, windef); +} + +/* + * transformDefineClause Process DEFINE clause and transform ResTarget into + * list of TargetEntry. + * + * XXX we only support column reference in row pattern definition search + * condition, e.g. "price". <row pattern definition variable name>.<column + * reference> is not supported, e.g. "A.price". + */ +static List * +transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist) +{ + /* DEFINE variable name initials */ + static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz"; + + ListCell *lc, *l; + ResTarget *restarget, *r; + List *restargets; + char *name; + int initialLen; + int i; + + /* + * If Row Definition Common Syntax exists, DEFINE clause must exist. + * (the raw parser should have already checked it.) + */ + Assert(windef->rpCommonSyntax->rpDefs != NULL); + + /* + * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE + * per the SQL standard. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + bool found = false; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *)lfirst(lc); + name = strVal(a->lexpr); + + foreach(l, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *)lfirst(l); + + if (!strcmp(restarget->name, name)) + { + found = true; + break; + } + } + + if (!found) + { + /* + * "name" is missing. So create "name AS name IS TRUE" ResTarget + * node and add it to the temporary list. + */ + A_Const *n; + + restarget = makeNode(ResTarget); + n = makeNode(A_Const); + n->val.boolval.type = T_Boolean; + n->val.boolval.boolval = true; + n->location = -1; + restarget->name = pstrdup(name); + restarget->indirection = NIL; + restarget->val = (Node *)n; + restarget->location = -1; + restargets = lappend((List *)restargets, restarget); + } + } + + if (list_length(restargets) >= 1) + { + /* add missing DEFINEs */ + windef->rpCommonSyntax->rpDefs = list_concat(windef->rpCommonSyntax->rpDefs, + restargets); + list_free(restargets); + } + + /* + * Check for duplicate row pattern definition variables. The standard + * requires that no two row pattern definition variable names shall be + * equivalent. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *)lfirst(lc); + name = restarget->name; + + /* + * Add DEFINE expression (Restarget->val) to the targetlist as a + * TargetEntry if it does not exist yet. Planner will add the column + * ref var node to the outer plan's target list later on. This makes + * DEFINE expression could access the outer tuple while evaluating + * PATTERN. + * + * XXX: adding whole expressions of DEFINE to the plan.targetlist is + * not so good, because it's not necessary to evalute the expression + * in the target list while running the plan. We should extract the + * var nodes only then add them to the plan.targetlist. + */ + findTargetlistEntrySQL99(pstate, (Node *)restarget->val, targetlist, EXPR_KIND_RPR_DEFINE); + + /* + * Make sure that the row pattern definition search condition is a + * boolean expression. + */ + transformWhereClause(pstate, restarget->val, + EXPR_KIND_RPR_DEFINE, "DEFINE"); + + foreach(l, restargets) + { + char *n; + + r = (ResTarget *) lfirst(l); + n = r->name; + + if (!strcmp(n, name)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause", + name), + parser_errposition(pstate, exprLocation((Node *)r)))); + } + restargets = lappend(restargets, restarget); + } + list_free(restargets); + + /* + * Create list of row pattern DEFINE variable name's initial. + * We assign [a-z] to them (up to 26 variable names are allowed). + */ + restargets = NIL; + i = 0; + initialLen = strlen(defineVariableInitials); + + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + char initial[2]; + + restarget = (ResTarget *)lfirst(lc); + name = restarget->name; + + if (i >= initialLen) + { + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("number of row pattern definition variable names exceeds %d", initialLen), + parser_errposition(pstate, exprLocation((Node *)restarget)))); + } + initial[0] = defineVariableInitials[i++]; + initial[1] = '\0'; + wc->defineInitial = lappend(wc->defineInitial, makeString(pstrdup(initial))); + } + + return transformTargetList(pstate, windef->rpCommonSyntax->rpDefs, + EXPR_KIND_RPR_DEFINE); +} + +/* + * transformPatternClause + * Process PATTERN clause and return PATTERN clause in the raw parse tree + */ +static void +transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef) +{ + ListCell *lc, *l; + + /* + * Row Pattern Common Syntax clause exists? + */ + if (windef->rpCommonSyntax == NULL) + return; + + /* + * Primary row pattern variable names in PATTERN clause must appear in + * DEFINE clause as row pattern definition variable names. + */ + wc->patternVariable = NIL; + wc->patternRegexp = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + char *name; + char *regexp; + bool found = false; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *)lfirst(lc); + name = strVal(a->lexpr); + + foreach(l, windef->rpCommonSyntax->rpDefs) + { + ResTarget *restarget = (ResTarget *)lfirst(l); + + if (!strcmp(restarget->name, name)) + { + found = true; + break; + } + } + + if (!found) + { + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("primary row pattern variable name \"%s\" does not appear in DEFINE clause", + name), + parser_errposition(pstate, exprLocation((Node *)a)))); + } + wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name))); + regexp = strVal(lfirst(list_head(a->name))); + wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp))); + } +} + +/* + * transformMeasureClause + * Process MEASURE clause + * XXX MEASURE clause is not supported yet + */ +static List * +transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef) +{ + if (windef->rowPatternMeasures == NIL) + return NIL; + + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("%s","MEASURE clause is not supported yet"), + parser_errposition(pstate, exprLocation((Node *)windef->rowPatternMeasures)))); + return NIL; +} diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 64c582c344..18b58ac263 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -557,6 +557,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) case EXPR_KIND_COPY_WHERE: case EXPR_KIND_GENERATED_COLUMN: case EXPR_KIND_CYCLE_MARK: + case EXPR_KIND_RPR_DEFINE: /* okay */ break; @@ -1770,6 +1771,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink) case EXPR_KIND_VALUES: case EXPR_KIND_VALUES_SINGLE: case EXPR_KIND_CYCLE_MARK: + case EXPR_KIND_RPR_DEFINE: /* okay */ break; case EXPR_KIND_CHECK_CONSTRAINT: @@ -3149,6 +3151,8 @@ ParseExprKindName(ParseExprKind exprKind) return "GENERATED AS"; case EXPR_KIND_CYCLE_MARK: return "CYCLE"; + case EXPR_KIND_RPR_DEFINE: + return "DEFINE"; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index b3f0b6a137..2ff3699538 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2656,6 +2656,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location) case EXPR_KIND_CYCLE_MARK: errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; /* * There is intentionally no default: case here, so that the -- 2.25.1 ----Next_Part(Fri_Sep_22_14_16_40_2023_530)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v7-0003-Row-pattern-recognition-patch-planner.patch" ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2023-09-25 22:20 Nathan Bossart <[email protected]> parent: bt23nguyent <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Nathan Bossart @ 2023-09-25 22:20 UTC (permalink / raw) To: bt23nguyent <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] On Thu, Sep 21, 2023 at 11:18:00AM +0900, bt23nguyent wrote: > -basic_archive_configured(ArchiveModuleState *state) > +basic_archive_configured(ArchiveModuleState *state, char **logdetail) Could we do something more like GUC_check_errdetail() instead to maintain backward compatibility with v16? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2023-09-26 06:13 Daniel Gustafsson <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Daniel Gustafsson @ 2023-09-26 06:13 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: bt23nguyent <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] > On 26 Sep 2023, at 00:20, Nathan Bossart <[email protected]> wrote: > > On Thu, Sep 21, 2023 at 11:18:00AM +0900, bt23nguyent wrote: >> -basic_archive_configured(ArchiveModuleState *state) >> +basic_archive_configured(ArchiveModuleState *state, char **logdetail) > > Could we do something more like GUC_check_errdetail() instead to maintain > backward compatibility with v16? We'd still need something exported to call into which isn't in 16, so it wouldn't be more than optically backwards compatible since a module written for 17 won't compile for 16, or am I missing something? -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2023-10-13 02:25 Nathan Bossart <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Nathan Bossart @ 2023-10-13 02:25 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: bt23nguyent <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] On Tue, Sep 26, 2023 at 08:13:45AM +0200, Daniel Gustafsson wrote: >> On 26 Sep 2023, at 00:20, Nathan Bossart <[email protected]> wrote: >> >> On Thu, Sep 21, 2023 at 11:18:00AM +0900, bt23nguyent wrote: >>> -basic_archive_configured(ArchiveModuleState *state) >>> +basic_archive_configured(ArchiveModuleState *state, char **logdetail) >> >> Could we do something more like GUC_check_errdetail() instead to maintain >> backward compatibility with v16? > > We'd still need something exported to call into which isn't in 16, so it > wouldn't be more than optically backwards compatible since a module written for > 17 won't compile for 16, or am I missing something? I only mean that a module written for v16 could continue to be used in v17 without any changes. You are right that a module that uses this new functionality wouldn't compile for v16. But IMHO the interface is nicer, too, since module authors wouldn't need to worry about allocating the space for the string or formatting the message. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2023-10-13 09:02 Daniel Gustafsson <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Daniel Gustafsson @ 2023-10-13 09:02 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: bt23nguyent <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] > On 13 Oct 2023, at 04:25, Nathan Bossart <[email protected]> wrote: > > On Tue, Sep 26, 2023 at 08:13:45AM +0200, Daniel Gustafsson wrote: >>> On 26 Sep 2023, at 00:20, Nathan Bossart <[email protected]> wrote: >>> >>> On Thu, Sep 21, 2023 at 11:18:00AM +0900, bt23nguyent wrote: >>>> -basic_archive_configured(ArchiveModuleState *state) >>>> +basic_archive_configured(ArchiveModuleState *state, char **logdetail) >>> >>> Could we do something more like GUC_check_errdetail() instead to maintain >>> backward compatibility with v16? >> >> We'd still need something exported to call into which isn't in 16, so it >> wouldn't be more than optically backwards compatible since a module written for >> 17 won't compile for 16, or am I missing something? > > I only mean that a module written for v16 could continue to be used in v17 > without any changes. You are right that a module that uses this new > functionality wouldn't compile for v16. Sure, but that also means that few if any existing modules will be updated to provide this =). > But IMHO the interface is nicer, That's a more compelling reason IMO. I'm not sure if I prefer the GUC_check_errdetail-like approach better, I would for sure not be opposed to reviewing a version of the patch doing it that way. Tung Nguyen: are you interested in updating the patch along these lines suggested by Nathan? > since module authors wouldn't need to worry about allocating the space > for the string or formatting the message. Well, they still need to format it; and calling <new_api>_errdetail(msg), pstrdup(msg) or psprintf(msg) isn't a world of difference. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2024-01-05 23:03 Nathan Bossart <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Nathan Bossart @ 2024-01-05 23:03 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: bt23nguyent <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] On Fri, Oct 13, 2023 at 11:02:39AM +0200, Daniel Gustafsson wrote: > That's a more compelling reason IMO. I'm not sure if I prefer the > GUC_check_errdetail-like approach better, I would for sure not be opposed to > reviewing a version of the patch doing it that way. > > Tung Nguyen: are you interested in updating the patch along these lines > suggested by Nathan? I gave it a try. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v4-0001-add-support-for-emitting-errdetail-from-archive-m.patch (2.8K, ../../20240105230357.GA2425960@nathanxps13/2-v4-0001-add-support-for-emitting-errdetail-from-archive-m.patch) download | inline diff: From 7183005bb6786b63a5fd96ba5101849eb6f203e5 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Fri, 5 Jan 2024 17:01:10 -0600 Subject: [PATCH v4 1/1] add support for emitting errdetail from archive module check-configured callback --- contrib/basic_archive/basic_archive.c | 8 +++++++- src/backend/postmaster/pgarch.c | 8 +++++++- src/include/archive/archive_module.h | 8 ++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c index 804567e919..2c8721ebf6 100644 --- a/contrib/basic_archive/basic_archive.c +++ b/contrib/basic_archive/basic_archive.c @@ -161,7 +161,13 @@ check_archive_directory(char **newval, void **extra, GucSource source) static bool basic_archive_configured(ArchiveModuleState *state) { - return archive_directory != NULL && archive_directory[0] != '\0'; + if (archive_directory == NULL || archive_directory[0] == '\0') + { + arch_module_check_errdetail("basic_archive.archive_directory is not set."); + return false; + } + + return true; } /* diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 67693b0580..f663965d89 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -91,6 +91,7 @@ typedef struct PgArchData } PgArchData; char *XLogArchiveLibrary = ""; +char *arch_module_check_errdetail_string; /* ---------- @@ -408,12 +409,17 @@ pgarch_ArchiverCopyLoop(void) */ HandlePgArchInterrupts(); + /* Reset variables that might be set by the callback */ + arch_module_check_errdetail_string = NULL; + /* can't do anything if not configured ... */ if (ArchiveCallbacks->check_configured_cb != NULL && !ArchiveCallbacks->check_configured_cb(archive_module_state)) { ereport(WARNING, - (errmsg("archive_mode enabled, yet archiving is not configured"))); + (errmsg("archive_mode enabled, yet archiving is not configured"), + arch_module_check_errdetail_string ? + errdetail_internal("%s", arch_module_check_errdetail_string) : 0)); return; } diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h index fd59b9faf4..763af76e54 100644 --- a/src/include/archive/archive_module.h +++ b/src/include/archive/archive_module.h @@ -56,4 +56,12 @@ typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void); extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void); +/* Support for messages reported from archive module callbacks. */ + +extern PGDLLIMPORT char *arch_module_check_errdetail_string; + +#define arch_module_check_errdetail \ + pre_format_elog_string(errno, TEXTDOMAIN), \ + arch_module_check_errdetail_string = format_elog_string + #endif /* _ARCHIVE_MODULE_H */ -- 2.25.1 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2024-02-28 18:51 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Nathan Bossart @ 2024-02-28 18:51 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: bt23nguyent <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] On Fri, Jan 05, 2024 at 05:03:57PM -0600, Nathan Bossart wrote: > I gave it a try. Is there any interest in this? If not, I'll withdraw the commitfest entry. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2024-02-28 21:05 Daniel Gustafsson <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Daniel Gustafsson @ 2024-02-28 21:05 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: bt23nguyent <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] > On 28 Feb 2024, at 19:51, Nathan Bossart <[email protected]> wrote: > > On Fri, Jan 05, 2024 at 05:03:57PM -0600, Nathan Bossart wrote: >> I gave it a try. > > Is there any interest in this? If not, I'll withdraw the commitfest entry. I'm still interested, please leave it in and I'll circle around to it. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set @ 2024-02-28 21:06 Nathan Bossart <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Nathan Bossart @ 2024-02-28 21:06 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: bt23nguyent <[email protected]>; Alvaro Herrera <[email protected]>; [email protected] On Wed, Feb 28, 2024 at 10:05:26PM +0100, Daniel Gustafsson wrote: >> On 28 Feb 2024, at 19:51, Nathan Bossart <[email protected]> wrote: >> Is there any interest in this? If not, I'll withdraw the commitfest entry. > > I'm still interested, please leave it in and I'll circle around to it. Thanks, Daniel. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2024-02-28 21:06 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-09-20 12:14 Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Daniel Gustafsson <[email protected]> 2023-09-21 02:18 ` bt23nguyent <[email protected]> 2023-09-25 22:20 ` Nathan Bossart <[email protected]> 2023-09-26 06:13 ` Daniel Gustafsson <[email protected]> 2023-10-13 02:25 ` Nathan Bossart <[email protected]> 2023-10-13 09:02 ` Daniel Gustafsson <[email protected]> 2024-01-05 23:03 ` Nathan Bossart <[email protected]> 2024-02-28 18:51 ` Nathan Bossart <[email protected]> 2024-02-28 21:05 ` Daniel Gustafsson <[email protected]> 2024-02-28 21:06 ` Nathan Bossart <[email protected]> 2023-09-22 04:53 [PATCH v7 2/7] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[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