Mark all GUC variable as PGDLLIMPORT
Hi, I've been pinged many time over the years to either fix Windows compatibility or provide DLL for multiple extensions I'm maintaining. I've finally taken some time to setup a Windows build environment so I could take care of most of the problem, but not all (at least not in a satisfactory way). I've also been looking a bit around other extensions and I see that the #1 problem with compiling extensions on Windows is the lack of PGDLLIMPORT annotations, which is 99% of the time for a GUC. This topic has been raised multiple time over the years, and I don't see any objection to add such an annotation at least for all GUC variables (either the direct variables or the indirect variables set during the hook execution), so PFA a patch that takes care of all the GUC. I don't now if that's still an option at that point, but backporting to at least pg14 if that patch is accepted would be quite helpful. >From 10af6e965ace671aad13cae47ee1b475ac24f15a Mon Sep 17 00:00:00 2001 From: Julien Rouhaud Date: Sun, 22 Aug 2021 15:40:47 +0800 Subject: [PATCH v1] Add PGDLLIMPORT to all direct or indirect GUC variables --- src/backend/utils/misc/guc.c | 18 +++--- src/include/access/gin.h | 2 +- src/include/access/tableam.h | 4 +- src/include/access/toast_compression.h| 2 +- src/include/access/xact.h | 12 ++-- src/include/access/xlog.h | 78 +++ src/include/catalog/namespace.h | 2 +- src/include/catalog/storage.h | 2 +- src/include/commands/async.h | 2 +- src/include/commands/user.h | 2 +- src/include/commands/vacuum.h | 12 ++-- src/include/fmgr.h| 2 +- src/include/jit/jit.h | 20 +++--- src/include/libpq/auth.h | 4 +- src/include/libpq/libpq.h | 24 +++ src/include/libpq/pqcomm.h| 2 +- src/include/miscadmin.h | 22 +++ src/include/optimizer/geqo.h | 10 +-- src/include/optimizer/optimizer.h | 4 +- src/include/optimizer/planmain.h | 6 +- src/include/parser/parse_expr.h | 2 +- src/include/parser/parser.h | 4 +- src/include/pgstat.h | 6 +- src/include/pgtime.h | 2 +- src/include/postmaster/autovacuum.h | 30 - src/include/postmaster/bgwriter.h | 8 +-- src/include/postmaster/postmaster.h | 28 src/include/postmaster/syslogger.h| 10 +-- src/include/postmaster/walwriter.h| 4 +- src/include/replication/logicallauncher.h | 4 +- src/include/replication/syncrep.h | 2 +- src/include/replication/walreceiver.h | 6 +- src/include/replication/walsender.h | 6 +- src/include/storage/bufmgr.h | 20 +++--- src/include/storage/dsm_impl.h| 4 +- src/include/storage/fd.h | 2 +- src/include/storage/large_object.h| 2 +- src/include/storage/lock.h| 12 ++-- src/include/storage/lwlock.h | 2 +- src/include/storage/pg_shmem.h| 6 +- src/include/storage/predicate.h | 6 +- src/include/storage/proc.h| 2 +- src/include/storage/standby.h | 8 +-- src/include/tcop/tcopprot.h | 6 +- src/include/tsearch/ts_cache.h| 2 +- src/include/utils/array.h | 2 +- src/include/utils/builtins.h | 2 +- src/include/utils/bytea.h | 2 +- src/include/utils/elog.h | 10 +-- src/include/utils/guc.h | 62 +- src/include/utils/pg_locale.h | 8 +-- src/include/utils/plancache.h | 2 +- src/include/utils/ps_status.h | 2 +- src/include/utils/queryjumble.h | 2 +- src/include/utils/rls.h | 2 +- src/include/utils/xml.h | 4 +- 56 files changed, 256 insertions(+), 256 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index a2e0f8de7e..145aeed94b 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -129,20 +129,20 @@ #define REALTYPE_PRECISION 17 /* XXX these should appear in other modules' header files */ -extern bool Log_disconnections; -extern int CommitDelay; -extern int CommitSiblings; -extern char *default_tablespace; -extern char *temp_tablespaces; -extern bool ignore_checksum_failure; -extern bool ignore_invalid_pages; +extern PGDLLIMPORT bool Log_disconnections; +extern PGDLLIMPORT int CommitDelay; +extern PGDLLIMPORT int CommitSiblings; +extern PGDLLIMPORT char *default_tablespace; +extern PGDLLIMPORT char *temp_tablespaces; +extern PGDLLIMPORT bool ignore_checksum_failure; +extern PGDLLIMPORT bool ignore_invalid_pages; extern bool synchronize_seqscans; #ifdef T
Re: Hook for extensible parsing.
On Thu, Jul 22, 2021 at 03:04:19PM +0800, Julien Rouhaud wrote: > On Thu, Jul 22, 2021 at 12:01:34PM +0530, vignesh C wrote: > > > > 1) CFBOT showed the following compilation errors in windows: > > Thanks for looking at it. I'm aware of this issue on windows, but as > mentioned > in the thread the new contrib is there to demonstrate how the new > infrastructure works. If there were some interest in pushing the patch, I > don't think that we would add a full bison parser, whether it's in contrib or > test modules. > > So unless there's a clear indication from a committer that we would want to > integrate such a parser, or if someone is interested in reviewing the patch > and > only has a windows machine, I don't plan to spend time trying to fix a windows > only problem for something that will disappear anyway. I'm not sure what changed in the Windows part of the cfbot, but somehow it's not hitting any compilation error anymore and all the tests are now green.
Re: Allow parallel DISTINCT
On Wed, 18 Aug 2021 at 08:50, Zhihong Yu wrote: > The patch is good from my point of view. Thanks for the review. I looked over the patch again and the only thing I adjusted was the order of the RESETs in the regression tests. I left the " if (distinct_rel->pathlist == NIL)" ERROR case check so that the ERROR is raised before we call the FDW function and hook function to add more paths. Part of me thinks it should probably go afterwards, but I didn't want to change the behaviour there. The other part of me thinks that if you can't do distinct by sorting or hashing then there's not much hope for the hook to add any paths either. I've pushed this to master now. David
Re: Mark all GUC variable as PGDLLIMPORT
On Sun, Aug 22, 2021 at 04:10:33PM +0800, Julien Rouhaud wrote: > This topic has been raised multiple time over the years, and I don't see any > objection to add such an annotation at least for all GUC variables (either the > direct variables or the indirect variables set during the hook execution), so > PFA a patch that takes care of all the GUC. > > I don't now if that's still an option at that point, but backporting to at > least pg14 if that patch is accepted would be quite helpful. These are usually just applied on HEAD, and on a parameter-basis based on requests from extension authors. If you wish to make your extensions able to work on Windows, that's a good idea, but I would recommend to limit this exercise to what's really necessary for your purpose. -- Michael signature.asc Description: PGP signature
Re: Mark all GUC variable as PGDLLIMPORT
On Sun, Aug 22, 2021 at 08:51:26PM +0900, Michael Paquier wrote: > On Sun, Aug 22, 2021 at 04:10:33PM +0800, Julien Rouhaud wrote: > > This topic has been raised multiple time over the years, and I don't see any > > objection to add such an annotation at least for all GUC variables (either > > the > > direct variables or the indirect variables set during the hook execution), > > so > > PFA a patch that takes care of all the GUC. > > > > I don't now if that's still an option at that point, but backporting to at > > least pg14 if that patch is accepted would be quite helpful. > > These are usually just applied on HEAD Yeah but 14 isn't released yet, and this is a really low risk change. > , and on a parameter-basis based > on requests from extension authors. If you wish to make your > extensions able to work on Windows, that's a good idea, but I would > recommend to limit this exercise to what's really necessary for your > purpose. I disagree. For random global variables I agree that we shouldn't mark them all blindly, but for GUCs it's pretty clear that they're intended to be accessible from any caller, including extensions. Why treating Windows as a second-class citizen, especially when any change can only be used a year after someone complained?
Re: Mark all GUC variable as PGDLLIMPORT
ne 22. 8. 2021 v 14:08 odesílatel Julien Rouhaud napsal: > On Sun, Aug 22, 2021 at 08:51:26PM +0900, Michael Paquier wrote: > > On Sun, Aug 22, 2021 at 04:10:33PM +0800, Julien Rouhaud wrote: > > > This topic has been raised multiple time over the years, and I don't > see any > > > objection to add such an annotation at least for all GUC variables > (either the > > > direct variables or the indirect variables set during the hook > execution), so > > > PFA a patch that takes care of all the GUC. > > > > > > I don't now if that's still an option at that point, but backporting > to at > > > least pg14 if that patch is accepted would be quite helpful. > > > > These are usually just applied on HEAD > > Yeah but 14 isn't released yet, and this is a really low risk change. > > > , and on a parameter-basis based > > on requests from extension authors. If you wish to make your > > extensions able to work on Windows, that's a good idea, but I would > > recommend to limit this exercise to what's really necessary for your > > purpose. > > I disagree. For random global variables I agree that we shouldn't mark > them > all blindly, but for GUCs it's pretty clear that they're intended to be > accessible from any caller, including extensions. Why treating Windows as > a > second-class citizen, especially when any change can only be used a year > after > someone complained? > I had few problems with access with these variables too when I worked on orafce. Is true, so it increases differences between Windows and Unix, and fixing these issues is not fun work. On the other hand, maybe direct access to these variables from extensions is an antipattern, and we should use a direct function call API and functions current_setting and set_config. The overhead is usually not too big.
Re: Mark all GUC variable as PGDLLIMPORT
On Sun, Aug 22, 2021 at 02:17:16PM +0200, Pavel Stehule wrote:
>
> Is true, so it increases differences between Windows and Unix, and fixing
> these issues is not fun work. On the other hand, maybe direct access to
> these variables from extensions is an antipattern, and we should use a
> direct function call API and functions current_setting and set_config. The
> overhead is usually not too big.
Yes, and that's what I did for one of my extensions. But that's still a bit of
overhead, and extra burden only seen when trying to have Windows compatiblity,
and I hope I can get rid of that at some point.
If direct variable access shouldn't be possible, then we should explicitly tag
those with __attribute__ ((visibility ("hidden"))) or something like that to
have a more consistent behavior.
Spelling change in LLVM 14 API
Hi, After [1], seawasp blew up[2]. I tested the following fix on LLVM 13 and 14 (main branch ~2 days ago). Better ideas welcome. - if (F.getAttributes().hasFnAttribute(llvm::Attribute::NoInline)) +#if LLVM_VERSION_MAJOR < 14 +#define hasFnAttr hasFnAttribute +#endif + + if (F.getAttributes().hasFnAttr(llvm::Attribute::NoInline)) [1] https://github.com/llvm/llvm-project/commit/92ce6db9ee7666a347fccf0f72ba3225b199d6d1 [2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2021-08-21%2023%3A17%3A30
Re: Mark all GUC variable as PGDLLIMPORT
Julien Rouhaud writes: > On Sun, Aug 22, 2021 at 08:51:26PM +0900, Michael Paquier wrote: >> ... and on a parameter-basis based >> on requests from extension authors. If you wish to make your >> extensions able to work on Windows, that's a good idea, but I would >> recommend to limit this exercise to what's really necessary for your >> purpose. > I disagree. For random global variables I agree that we shouldn't mark them > all blindly, but for GUCs it's pretty clear that they're intended to be > accessible from any caller, including extensions. Uh, no, it's exactly *not* clear. There are a lot of GUCs that are only of interest to particular subsystems. I do not see why being a GUC makes something automatically more interesting than any other global variable. Usually, the fact that one is global is only so the GUC machinery itself can get at it, otherwise it'd be static in the owning module. As for "extensions should be able to get at the values", the GUC machinery already provides uniform mechanisms for doing that safely. Direct access to the variable's internal value would be unsafe in many cases. regards, tom lane
Re: Spelling change in LLVM 14 API
Thomas Munro writes: > After [1], seawasp blew up[2]. I tested the following fix on LLVM 13 > and 14 (main branch ~2 days ago). Better ideas welcome. > - if (F.getAttributes().hasFnAttribute(llvm::Attribute::NoInline)) > +#if LLVM_VERSION_MAJOR < 14 > +#define hasFnAttr hasFnAttribute > +#endif > + > + if (F.getAttributes().hasFnAttr(llvm::Attribute::NoInline)) Seems like either we should push back on pointless renaming, or else that we're not really supposed to be accessing this non-stable API. I have no idea which of those situations applies ... but if the LLVM guys are randomly renaming methods that *are* supposed to be user-visible, they need re-education. regards, tom lane
Re: Mark all GUC variable as PGDLLIMPORT
On Sun, Aug 22, 2021 at 09:19:42AM -0400, Tom Lane wrote: > > Uh, no, it's exactly *not* clear. There are a lot of GUCs that are only > of interest to particular subsystems. I do not see why being a GUC makes > something automatically more interesting than any other global variable. > Usually, the fact that one is global is only so the GUC machinery itself > can get at it, otherwise it'd be static in the owning module. > > As for "extensions should be able to get at the values", the GUC machinery > already provides uniform mechanisms for doing that safely. Direct access > to the variable's internal value would be unsafe in many cases. Then shouldn't we try to prevent direct access on all platforms rather than only one?
Re: Allow parallel DISTINCT
David Rowley writes: > I've pushed this to master now. ... and the buildfarm is pushing back, eg https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2021-08-22%2011%3A31%3A45 diff -U3 /scratch/nm/farm/xlc64v16/HEAD/pgsql.build/src/test/regress/expected/select_distinct.out /scratch/nm/farm/xlc64v16/HEAD/pgsql.build/src/test/regress/results/select_distinct.out --- /scratch/nm/farm/xlc64v16/HEAD/pgsql.build/src/test/regress/expected/select_distinct.out 2021-08-22 11:26:22.0 + +++ /scratch/nm/farm/xlc64v16/HEAD/pgsql.build/src/test/regress/results/select_distinct.out 2021-08-22 11:38:43.0 + @@ -223,7 +223,7 @@ -> Sort Sort Key: four -> Gather - Workers Planned: 2 + Workers Planned: 5 -> HashAggregate Group Key: four -> Parallel Seq Scan on tenk1 @@ -270,7 +270,7 @@ -> Sort Sort Key: (distinct_func(1)) -> Gather - Workers Planned: 2 + Workers Planned: 5 -> Parallel Seq Scan on tenk1 (6 rows) regards, tom lane
Re: Allow parallel DISTINCT
On Mon, 23 Aug 2021 at 01:58, Tom Lane wrote: > > David Rowley writes: > > I've pushed this to master now. > > ... and the buildfarm is pushing back, eg Thanks. I pushed a fix for that. David
Re: Allow parallel DISTINCT
David Rowley writes: > Thanks. I pushed a fix for that. Yeah, I saw your commit just after complaining. Sorry for the noise. regards, tom lane
Queries that should be canceled will get stuck on secure_write function
Hi, all Recently, I got a problem that the startup process of standby is stuck and keeps in a waiting state. The backtrace of startup process shows that it is waiting for a backend process which conflicts with recovery processing to exit, the guc parameters max_standby_streaming_delay and max_standby_archive_delay are both set as 30 seconds, but it doesn't work. The backend process keeps alive, and the backtrace of this backend process show that it is waiting for the socket to be writeable in secure_write(). After further reading the code, I found that ProcessClientWriteInterrupt() in secure_write() only process interrupts when ProcDiePending is true, otherwise do nothing. However, snapshot conflicts with recovery will only set QueryCancelPending as true, so the response to the signal will de delayed indefinitely if the corresponding client is stuck, thus blocking the recovery process. I want to know why the interrupt is only handled when ProcDiePending is true, I think query which is supposed to be canceled also should respond to the signal. I also want to share a patch with you, I add a guc parameter max_standby_client_write_delay, if a query is supposed to be canceled, and the time delayed by a client exceeds max_standby_client_write_delay, then set ProcDiePending as true to avoid being delayed indefinitely, what do you think of it, hope to get your reply. Thanks & Best Regard 0001-Set-max-client-write-delay-timeout-for-query-which-i.patch Description: Binary data
Re: Support reset of Shared objects statistics in "pg_stat_reset" function
> 3) > - dbentry = pgstat_get_db_entry(msg->m_databaseid, false); > + if (!IsSharedRelation(msg->m_objectid)) > + dbentry = pgstat_get_db_entry(msg->m_databaseid, false); > + else > + dbentry = pgstat_get_db_entry(InvalidOid, false); > > We should add some comments before this change. In my opinion, the comments added above the function "pgstat_recv_resetsinglecounter" and the function call "IsSharedRelation" added are self explanatory. If we add anything more, it will be a duplication. So No need to add any more comments here. Thanks & Regards SadhuPrasad EnterpriseDB: http://www.enterprisedb.com
Re: Support reset of Shared objects statistics in "pg_stat_reset" function
> On 2021/08/20 11:07, Mahendra Singh Thalor wrote: > > 1) > > Resets statistics for a single table or index in the current > > database > > -to zero. > > +to zero. The input can be a shared table also > > > > I think, above comment should be modified. Maybe, we can modify it as "If > > input is a shared oid(table or index or toast), then resets statistics for > > a single shared entry to zero. > > I'm not sure if ordinary users can understand what "shared oid" means. > Instead, > what about "Resets statistics for a single relation in the current database or > shared across all databases in the cluster to zero."? > Thank you for the review here. As per the comments, attached the latest patch here... Thanks & Regards SadhuPrasad EnterpriseDB: http://www.enterprisedb.com v5-0001-Enhancing-pg_stat_reset_single_table_counters-to-.patch Description: Binary data
Re: proposal: enhancing plpgsql debug API - returns text value of variable content
Hi
st 28. 7. 2021 v 11:01 odesílatel Pavel Stehule
napsal:
>
>
> pá 23. 7. 2021 v 10:47 odesílatel Pavel Stehule
> napsal:
>
>>
>>
>> pá 23. 7. 2021 v 10:30 odesílatel Aleksander Alekseev <
>> aleksan...@timescale.com> napsal:
>>
>>> Hi Pavel,
>>>
>>> > I know it. Attached patch try to fix this issue
>>> >
>>> > I merged you patch (thank you)
>>>
>>> Thanks! I did some more minor changes, mostly in the comments. See the
>>> attached patch. Other than that it looks OK. I think it's Ready for
>>> Committer now.
>>>
>>
>> looks well,
>>
>> thank you very much
>>
>> Pavel
>>
>
> rebase
>
unfortunately, previous patch that I sent was broken, so I am sending fixed
patch and fresh rebase
Regards
Pavel
> Pavel
>
>
>>
>>> --
>>> Best regards,
>>> Aleksander Alekseev
>>>
>>
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 7c5bc63778..77bceec7d9 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4072,6 +4072,8 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
{
(*plpgsql_plugin_ptr)->error_callback = plpgsql_exec_error_callback;
(*plpgsql_plugin_ptr)->assign_expr = exec_assign_expr;
+ (*plpgsql_plugin_ptr)->eval_datum = exec_eval_datum;
+ (*plpgsql_plugin_ptr)->cast_value = do_cast_value;
if ((*plpgsql_plugin_ptr)->func_setup)
((*plpgsql_plugin_ptr)->func_setup) (estate, func);
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 0f6a5b30b1..abe7d63f78 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -415,6 +415,7 @@ pl_block : decl_sect K_BEGIN proc_sect exception_sect K_END opt_label
new->cmd_type = PLPGSQL_STMT_BLOCK;
new->lineno = plpgsql_location_to_lineno(@2);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->label = $1.label;
new->n_initvars = $1.n_initvars;
new->initvarnos = $1.initvarnos;
@@ -907,7 +908,8 @@ stmt_perform : K_PERFORM
new = palloc0(sizeof(PLpgSQL_stmt_perform));
new->cmd_type = PLPGSQL_STMT_PERFORM;
new->lineno = plpgsql_location_to_lineno(@1);
- new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
plpgsql_push_back_token(K_PERFORM);
/*
@@ -943,6 +945,7 @@ stmt_call : K_CALL
new->cmd_type = PLPGSQL_STMT_CALL;
new->lineno = plpgsql_location_to_lineno(@1);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
plpgsql_push_back_token(K_CALL);
new->expr = read_sql_stmt();
new->is_call = true;
@@ -962,6 +965,7 @@ stmt_call : K_CALL
new->cmd_type = PLPGSQL_STMT_CALL;
new->lineno = plpgsql_location_to_lineno(@1);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
plpgsql_push_back_token(K_DO);
new->expr = read_sql_stmt();
new->is_call = false;
@@ -1000,7 +1004,8 @@ stmt_assign : T_DATUM
new = palloc0(sizeof(PLpgSQL_stmt_assign));
new->cmd_type = PLPGSQL_STMT_ASSIGN;
new->lineno = plpgsql_location_to_lineno(@1);
- new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->varno = $1.datum->dno;
/* Push back the head name to include it in the stmt */
plpgsql_push_back_token(T_DATUM);
@@ -1022,6 +1027,7 @@ stmt_getdiag : K_GET getdiag_area_opt K_DIAGNOSTICS getdiag_list ';'
new->cmd_type = PLPGSQL_STMT_GETDIAG;
new->lineno = plpgsql_location_to_lineno(@1);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->is_stacked = $2;
new->diag_items = $4;
@@ -1194,6 +1200,7 @@ stmt_if : K_IF expr_until_then proc_sect stmt_elsifs stmt_else K_END K_IF ';'
new->cmd_type = PLPGSQL_STMT_IF;
new->lineno = plpgsql_location_to_lineno(@1);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->cond = $2;
new->then_body = $3;
new->elsif_list = $4;
@@ -1299,6 +1306,7 @@ stmt_loop : opt_loop_label K_LOOP loop_body
new->cmd_type = PLPGSQL_STMT_LOOP;
new->lineno = plpgsql_location_to_lineno(@2);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->label = $1;
new->body = $3.stmts;
@@ -1317,6 +1325,7 @@ stmt_while : opt_loop_label K_WHILE expr_until_loop loop_body
new->cmd_type = PLPGSQL_STMT_WHILE;
new->lineno = plpgsql_location_to_lineno(@2);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->label = $1;
new->cond = $3;
new->body = $4.stmts;
@@ -1381,6 +1390,7 @@ for_control : for_variable K_IN
new = palloc0(size
Re: replay of CREATE TABLESPACE eats data at wal_level=minimal
On Wed, Aug 18, 2021 at 10:32:10PM -0700, Noah Misch wrote:
> On Wed, Aug 18, 2021 at 10:47:24AM -0400, Robert Haas wrote:
> > On Tue, Aug 10, 2021 at 9:35 AM Robert Haas wrote:
> > > Oh, yeah, I think that works, actually. I was imagining a few problems
> > > here, but I don't think they really exist. The redo routines for files
> > > within the directory can't possibly care about having the old files
> > > erased for them, since that wouldn't be something that would normally
> > > happen, if there were no recent CREATE TABLESPACE involved. And
> > > there's code further down to remove and recreate the symlink, just in
> > > case. So I think your proposed patch might be all we need.
> >
> > Noah, do you plan to commit this?
>
> Yes. I feel it needs a test case, which is the main reason I've queued the
> task rather than just pushed what I posted last.
Here's what I plan to push. Besides adding a test, I modified things so
CREATE TABLESPACE redo continues to report an error if a non-directory exists
under the name we seek to create. One could argue against covering that
corner case, but TablespaceCreateDbspace() does cover it.
Author: Noah Misch
Commit: Noah Misch
Fix data loss in wal_level=minimal crash recovery of CREATE TABLESPACE.
If the system crashed between CREATE TABLESPACE and the next checkpoint,
the result could be some files in the tablespace unexpectedly containing
no rows. Affected files would be those for which the system did not
write WAL; see the wal_skip_threshold documentation. Before v13, a
different set of conditions governed the writing of WAL; see v12's
. Users may want to audit non-default
tablespaces for unexpected short files. This bug could have truncated
an index without affecting the associated table, and reindexing the
index would fix that particular problem. This fixes the bug by making
create_tablespace_directories() more like TablespaceCreateDbspace().
Back-patch to 9.6 (all supported versions).
Reviewed by Robert Haas.
Discussion:
https://postgr.es/m/CA+TgmoaLO9ncuwvr2nN-J4VEP5XyAcy=zkihxqzbbfrxxgx...@mail.gmail.com
diff --git a/src/backend/commands/tablespace.c
b/src/backend/commands/tablespace.c
index a54239a..932e7ae 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -614,40 +614,36 @@ create_tablespace_directories(const char *location, const
Oid tablespaceoid)
location)));
}
- if (InRecovery)
- {
- /*
-* Our theory for replaying a CREATE is to forcibly drop the
target
-* subdirectory if present, and then recreate it. This may be
more
-* work than needed, but it is simple to implement.
-*/
- if (stat(location_with_version_dir, &st) == 0 &&
S_ISDIR(st.st_mode))
- {
- if (!rmtree(location_with_version_dir, true))
- /* If this failed, MakePGDirectory() below is
going to error. */
- ereport(WARNING,
- (errmsg("some useless files may
be left behind in old database directory \"%s\"",
-
location_with_version_dir)));
- }
- }
-
/*
* The creation of the version directory prevents more than one
tablespace
-* in a single location.
+* in a single location. This imitates TablespaceCreateDbspace(), but
it
+* ignores concurrency and missing parent directories. The chmod()
would
+* have failed in the absence of a parent. pg_tablespace_spcname_index
+* prevents concurrency.
*/
- if (MakePGDirectory(location_with_version_dir) < 0)
+ if (stat(location_with_version_dir, &st) < 0)
{
- if (errno == EEXIST)
+ if (errno != ENOENT)
ereport(ERROR,
- (errcode(ERRCODE_OBJECT_IN_USE),
-errmsg("directory \"%s\" already in
use as a tablespace",
+ (errcode_for_file_access(),
+errmsg("could not stat directory
\"%s\": %m",
location_with_version_dir)));
- else
+ else if (MakePGDirectory(location_with_version_dir) < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not create directory
\"%s\": %m",
location_with_version_dir)));
}
+ else if (!S_ISDIR(st.st_mode))
+ ereport(ERROR,
+
Improved regular expression error message for backrefs
Hackers, Please find attached an improvement to the error messages given for invalid backreference usage: select 'xyz' ~ '(.)(.)\3'; ERROR: invalid regular expression: invalid backreference number select 'xyz' ~ '(.)(.)(?=\2)'; -ERROR: invalid regular expression: invalid backreference number +ERROR: invalid regular expression: backreference in lookaround assertion The first regexp is invalid because only two capture groups exist, so \3 doesn't refer to anything. The second regexp is rejected because the regular expression system does not support backreferences within lookaround assertions. (See the docs, section 9.7.3.6. Limits And Compatibility.) It is flat wrong to say the backreference number is invalid. There is a perfectly valid capture that \2 refers to. The patch defines a new error code REG_ENOBREF in regex/regex.h right next to REG_ESUBREG from which it is split out, rather than at the end of the list. Is there a project preference to add it at the end? Certainly, that would give a shorter git diff. Are there dependencies on the current error messages which prevent such changes? v1-0001-Distinguishing-regular-expression-backref-errors.patch Description: Binary data — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Showing I/O timings spent reading/writing temp buffers in EXPLAIN
On Thu, Aug 19, 2021 at 10:52 PM Ranier Vilela wrote: > > Em qui., 19 de ago. de 2021 às 09:21, Masahiko Sawada > escreveu: >> >> Hi all , >> >> It's reported on pgsql-bugs[1] that I/O timings in EXPLAIN don't show >> the one for temp files. I think it's not a bug but could be an item >> for PG15. As mentioned on that thread, this would be useful for users >> in a case where temp buffers I/O used most of the time. So I've >> written the patch for that. Please note that the patch includes only >> to show temp buffer I/O timing to EXPLAIN but not other possibly >> related changes such as pg_stat_statement improvements yet. >> >> Before (w/o patch): >> postgres(1:14101)=# explain (analyze, buffers) select count(*) from >> generate_series(1,10); >> QUERY PLAN >> --- >> Aggregate (cost=1250.00..1250.01 rows=1 width=8) (actual >> time=59.025..59.026 rows=1 loops=1) >>Buffers: temp read=171 written=171 >>-> Function Scan on generate_series (cost=0.00..1000.00 >> rows=10 width=0) (actual time=21.695..45.524 rows=10 loops=1) >> Buffers: temp read=171 written=171 >> Planning Time: 0.041 ms >> Execution Time: 70.867 ms >> (6 rows) >> >> After (w/ patch): >> postgres(1:28754)=# explain (analyze, buffers) select count(*) from >> generate_series(1,10); >> QUERY PLAN >> --- >> Aggregate (cost=1250.00..1250.01 rows=1 width=8) (actual >> time=56.189..56.190 rows=1 loops=1) >>Buffers: temp read=171 written=171 >>I/O Timings: temp read=0.487 write=2.073 >>-> Function Scan on generate_series (cost=0.00..1000.00 >> rows=10 width=0) (actual time=21.072..42.886 rows=10 loops=1) >> Buffers: temp read=171 written=171 >> I/O Timings: temp read=0.487 write=2.073 >> Planning Time: 0.041 ms >> Execution Time: 59.928 ms >> (8 rows) >> >> Feedback is very welcome. > Thank you for the comments! > The presentation seems a little confusing, wouldn't it be better? > > I/O Timings: shared/local read= write=xxx temp read=0.487 write=2.073 Yeah, it looks better to add "shared/local". > > I think can remove this lines: > + if (has_temp_timing) > + appendStringInfoChar(es->str, ','); But I think that it's consistent with buffers statistics in EXPLAIN command. For example, "Buffers" in the output of EXPLAIN command could be like: Buffers: shared hit=1398, temp read=526 written=526 Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: .ready and .done files considered harmful
On 8/21/21, 9:29 PM, "Bossart, Nathan" wrote: > I was curious about this, so I wrote a patch (attached) to store > multiple files per directory scan and tested it against the latest > patch in this thread (v9) [0]. Specifically, I set archive_command to > 'false', created ~20K WAL segments, then restarted the server with > archive_command set to 'true'. Both the v9 patch and the attached > patch completed archiving all segments in just under a minute. (I > tested the attached patch with NUM_FILES_PER_DIRECTORY_SCAN set to 64, > 128, and 256 and didn't observe any significant difference.) The > existing logic took over 4 minutes to complete. > > I'm hoping to do this test again with many more (100K+) status files, > as I believe that the v9 patch will be faster at that scale, but I'm > not sure how much faster it will be. I ran this again on a bigger machine with 200K WAL files pending archive. The v9 patch took ~5.5 minutes, the patch I sent took ~8 minutes, and the existing logic took just under 3 hours. Nathan
Re: Improved regular expression error message for backrefs
Mark Dilger writes: > The patch defines a new error code REG_ENOBREF in regex/regex.h right next to > REG_ESUBREG from which it is split out, rather than at the end of the list. > Is there a project preference to add it at the end? Certainly, that would > give a shorter git diff. > Are there dependencies on the current error messages which prevent such > changes? Yeah: the POSIX standard says what the error codes from regcomp() are. POSIX defines REG_ESUBREG Number in \digit invalid or in error. which does seem to cover this case, so what I'd argue is that we should improve the "invalid backreference number" text rather than invent a nonstandard error code. Maybe about like "backreference number does not exist or cannot be referenced from here"? (Admittedly, there's not a huge reason why src/backend/regex/ needs to stay compliant with the POSIX API today. But I still have ambitions to split that out as a free-standing library someday, as Henry Spencer had originally planned to do. So I'd rather stick to the API spec.) It might be worth checking what text is attributed to this error code by PCRE and other implementations of the POSIX spec. regards, tom lane
Re: Is it worth pushing conditions to sublink/subplan?
Indeed, this may be useful for partition pruning. I am also curious about why this has not been achieved. Wenjing 于2021年8月23日周一 上午10:46写道： > Hi Hackers, > > Recently, a issue has been bothering me, This is about conditional > push-down in SQL. > I use cases from regression testing as an example. > I found that the conditions (B =1) can be pushed down into the > subquery, However, it cannot be pushed down to sublink/subplan. > If a sublink/subplan clause contains a partition table, it can be useful > to get the conditions for pruning. > So, is it worth pushing conditions to sublink/subplan? > Anybody have any ideas? > > > regards, > Wenjing > > > example: > create table p (a int, b int, c int) partition by list (a); > create table p1 partition of p for values in (1); > create table p2 partition of p for values in (2); > create table q (a int, b int, c int) partition by list (a); > create table q1 partition of q for values in (1) partition by list (b); > create table q11 partition of q1 for values in (1) partition by list (c); > create table q111 partition of q11 for values in (1); > create table q2 partition of q for values in (2) partition by list (b); > create table q21 partition of q2 for values in (1); > create table q22 partition of q2 for values in (2); > insert into q22 values (2, 2, 3); > > > postgres-# explain (costs off) > postgres-# select temp.b from > postgres-# ( > postgres(# select a,b from ab x where x.a = 1 > postgres(# union all > postgres(# (values(1,1)) > postgres(# ) temp, > postgres-# ab y > postgres-# where y.b = temp.b and y.a = 1 and y.b=1; > QUERY PLAN > --- > Nested Loop >-> Seq Scan on ab_a1_b1 y > Filter: ((b = 1) AND (a = 1)) >-> Append > -> Subquery Scan on "*SELECT* 1" >-> Seq Scan on ab_a1_b1 x > Filter: ((a = 1) AND (b = 1)) > -> Result > (8 rows) > > The conditions (B =1) can be pushed down into the subquery. > > postgres=# explain (costs off) > postgres-# select > postgres-# y.a, > postgres-# (Select x.b from ab x where y.a =x.a and y.b=x.b) as b > postgres-# from ab y where a = 1 and b = 1; > QUERY PLAN > --- > Seq Scan on ab_a1_b1 y >Filter: ((a = 1) AND (b = 1)) >SubPlan 1 > -> Append >-> Seq Scan on ab_a1_b1 x_1 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a1_b2 x_2 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a1_b3 x_3 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a2_b1 x_4 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a2_b2 x_5 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a2_b3 x_6 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a3_b1 x_7 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a3_b2 x_8 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a3_b3 x_9 > Filter: ((y.a = a) AND (y.b = b)) > (22 rows) > > The conditions (B = 1 and A = 1) cannot be pushed down to sublink/subplan > in targetlist. > > postgres=# explain (costs off) > postgres-# select y.a > postgres-# from ab y > postgres-# where > postgres-# (select x.a > x.b from ab x where y.a =x.a and y.b=x.b) and > postgres-# y.a = 1 and y.b = 1; > QUERY PLAN > --- > Seq Scan on ab_a1_b1 y >Filter: ((a = 1) AND (b = 1) AND (SubPlan 1)) >SubPlan 1 > -> Append >-> Seq Scan on ab_a1_b1 x_1 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a1_b2 x_2 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a1_b3 x_3 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a2_b1 x_4 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a2_b2 x_5 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a2_b3 x_6 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a3_b1 x_7 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a3_b2 x_8 > Filter: ((y.a = a) AND (y.b = b)) >-> Seq Scan on ab_a3_b3 x_9 > Filter: ((y.a = a) AND (y.b = b)) > (22 rows) > > The conditions (B=1 and A=1) cannot be pushed down to sublink/subplan in > where clause. > > > >
Re: Skipping logical replication transactions on subscriber side
On Fri, Aug 20, 2021 at 6:14 PM tanghy.f...@fujitsu.com wrote: > > > On Thursday, August 19, 2021 9:53 AM Masahiko Sawada > > wrote: > > > > Thank you for reporting the issue! This issue must be fixed in the > > latest (v9) patches I've just submitted[1]. > > > > Thanks for your patch. > I've confirmed the issue is fixed as you said. Thanks for your confirmation! Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Improved regular expression error message for backrefs
> On Aug 22, 2021, at 7:47 PM, Tom Lane wrote: > > Yeah: the POSIX standard says what the error codes from regcomp() are. I'm not sure how to interpret them. The language "The implementation may define additional macros or constants using names beginning with REG_" at the bottom of the docs might imply that one can add to the list. > POSIX defines > > REG_ESUBREG > Number in \digit invalid or in error. > > which does seem to cover this case, Hmm. The number is neither invalid nor in error. The only thing arguing in favor of using this code is that the error message contains the word "backreference": "REG_ESUBREG", "invalid backreference number" which gives the reader a clue that the problem has something to do with a backreference in the pattern. But the POSIX wording "Number in \digit invalid or in error." doesn't even have that advantage. We seem to be using the wrong return code. I would think a more generic REG_BADPAT Invalid regular expression. would be the correct code, though arguably far less informative. > so what I'd argue is that we should > improve the "invalid backreference number" text rather than invent > a nonstandard error code. Maybe about like "backreference number does > not exist or cannot be referenced from here"? Assuming we leave the error codes alone, how about, "backreference number invalid or cannot be referenced from here"? > (Admittedly, there's not a huge reason why src/backend/regex/ needs to > stay compliant with the POSIX API today. But I still have ambitions to > split that out as a free-standing library someday, as Henry Spencer had > originally planned to do. So I'd rather stick to the API spec.) That's fine. Something else might kill that ambition, but this quibble over error messages isn't nearly important enough to do so. > It might be worth checking what text is attributed to this error code > by PCRE and other implementations of the POSIX spec. Reading the docs at pcre.org, it appears that capture groups are allowed in look-around assertions. Our engine doesn't do that, instead treating all groups within assertions as non-capturing. I don't see anything about whether backreferences are allowed within pcre assertions, but I know that perl itself does allow them. So maybe the error text used by other implementations is irrelevant? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
RE: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
On Sat, Aug 21, 2021 8:38 PM Dilip Kumar wrote: > On Wed, Aug 18, 2021 at 3:45 PM Dilip Kumar wrote: > > I was looking into this, so if we want to do that I think the outline > > will look like this > > > > - There will be a fileset.c and fileset.h files, and we will expose a > > new structure FileSet, which will be the same as SharedFileSet, except > > mutext and refcount. The fileset.c will expose FileSetInit(), > > FileSetCreate(), FileSetOpen(), FileSetDelete() and FileSetDeleteAll() > > interfaces. > > > > - sharefileset.c will internally call the fileset.c's interfaces. The > > SharedFileSet structure will also contain FileSet and other members > > i.e. mutex and refcount. > > > > - the buffile.c's interfaces which are ending with Shared e.g. > > BufFileCreateShared, BufFileOpenShared, should be converted to > > BufFileCreate and BufFileOpen respectively. And the input to these > > interfaces can be converted to FileSet instead of SharedFileSet. > > Here is the first draft based on the idea we discussed, 0001, splits > sharedfileset.c in sharedfileset.c and fileset.c and 0002 is same patch I > submitted earlier(use single fileset throughout the worker), just it is > rebased on > top of 0001. Please let me know your thoughts. Hi, Here are some comments for the new version patches. 1) + TempTablespacePath(tempdirpath, tablespace); + snprintf(path, MAXPGPATH, "%s/%s%lu.%u.sharedfileset", +tempdirpath, PG_TEMP_FILE_PREFIX, +(unsigned long) fileset->creator_pid, fileset->number); do we need to use different filename for shared and un-shared fileset ? 2) I think we can remove or adjust the following comments in sharedfileset.c. * SharedFileSets can be used by backends when the temporary files need to be * opened/closed multiple times and the underlying files need to survive across * transactions. * We can also use this interface if the temporary files are used only by * single backend but the files need to be opened and closed multiple times * and also the underlying files need to survive across transactions. For 3) The 0002 patch still used the word "shared fileset" in some places, I think we should change it to "fileset". 4) -extern File SharedFileSetCreate(SharedFileSet *fileset, const char *name); -extern File SharedFileSetOpen(SharedFileSet *fileset, const char *name, - int mode); -extern bool SharedFileSetDelete(SharedFileSet *fileset, const char *name, - bool error_on_failure); extern void SharedFileSetDeleteAll(SharedFileSet *fileset); -extern void SharedFileSetUnregister(SharedFileSet *input_fileset); I noticed the patch delete part of public api, is it better to keep the old api and let them invoke new api internally ? Having said that, I didn’t find any open source extension use these old api, so it might be fine to delete them. Best regards, Hou zj
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
On Mon, Aug 23, 2021 at 9:11 AM houzj.f...@fujitsu.com wrote: > 4) > -extern File SharedFileSetCreate(SharedFileSet *fileset, const char *name); > -extern File SharedFileSetOpen(SharedFileSet *fileset, const char *name, > - int mode); > -extern bool SharedFileSetDelete(SharedFileSet *fileset, const char *name, > - bool > error_on_failure); > extern void SharedFileSetDeleteAll(SharedFileSet *fileset); > -extern void SharedFileSetUnregister(SharedFileSet *input_fileset); > > I noticed the patch delete part of public api, is it better to keep the old > api and > let them invoke new api internally ? Having said that, I didn’t find any open > source > extension use these old api, so it might be fine to delete them. Right, those were internally used by buffile.c but now we have changed buffile.c to directly use the fileset interfaces, so we better remove them. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com wrote: > > Personally, I also think it will be better to make the behavior consistent. > Attach the new version patch make both ADD and DROP behave the same as SET > PUBLICATION > which refresh all the publications. > I think we can have tests in the separate test file (alter_sub_pub.pl) like you earlier had in one of the versions. Use some meaningful names for tables instead of temp1, temp2 as you had in the previous version. Otherwise, the code changes look good to me. -- With Regards, Amit Kapila.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Wed, Aug 18, 2021 at 7:54 PM Peter Eisentraut wrote: > > On 10.08.21 05:22, Amit Kapila wrote: > > Yeah, unless we change design drastically we might not be able to do a > > refresh for dropped publications, for add it is possible. It seems > > most of the people responded on this thread that we can be consistent > > in terms of refreshing for add/drop at this stage but we can still go > > with another option where we can refresh only newly added publications > > for add but for drop refresh all publications. > > > > Peter E., do you have any opinion on this matter? > > Refresh everything for both ADD and DROP makes sense to me. > Thanks for your suggestion. -- With Regards, Amit Kapila.
Re: proposal: enhancing plpgsql debug API - returns text value of variable content
ne 22. 8. 2021 v 19:38 odesílatel Pavel Stehule
napsal:
> Hi
>
> st 28. 7. 2021 v 11:01 odesílatel Pavel Stehule
> napsal:
>
>>
>>
>> pá 23. 7. 2021 v 10:47 odesílatel Pavel Stehule
>> napsal:
>>
>>>
>>>
>>> pá 23. 7. 2021 v 10:30 odesílatel Aleksander Alekseev <
>>> aleksan...@timescale.com> napsal:
>>>
Hi Pavel,
> I know it. Attached patch try to fix this issue
>
> I merged you patch (thank you)
Thanks! I did some more minor changes, mostly in the comments. See the
attached patch. Other than that it looks OK. I think it's Ready for
Committer now.
>>>
>>> looks well,
>>>
>>> thank you very much
>>>
>>> Pavel
>>>
>>
>> rebase
>>
>
> unfortunately, previous patch that I sent was broken, so I am sending
> fixed patch and fresh rebase
>
This version set $contrib_extraincludes to fix windows build
Regards
Pavel
> Regards
>
> Pavel
>
>
>> Pavel
>>
>>
>>>
--
Best regards,
Aleksander Alekseev
>>>
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 7c5bc63778..77bceec7d9 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4072,6 +4072,8 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
{
(*plpgsql_plugin_ptr)->error_callback = plpgsql_exec_error_callback;
(*plpgsql_plugin_ptr)->assign_expr = exec_assign_expr;
+ (*plpgsql_plugin_ptr)->eval_datum = exec_eval_datum;
+ (*plpgsql_plugin_ptr)->cast_value = do_cast_value;
if ((*plpgsql_plugin_ptr)->func_setup)
((*plpgsql_plugin_ptr)->func_setup) (estate, func);
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 0f6a5b30b1..abe7d63f78 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -415,6 +415,7 @@ pl_block : decl_sect K_BEGIN proc_sect exception_sect K_END opt_label
new->cmd_type = PLPGSQL_STMT_BLOCK;
new->lineno = plpgsql_location_to_lineno(@2);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->label = $1.label;
new->n_initvars = $1.n_initvars;
new->initvarnos = $1.initvarnos;
@@ -907,7 +908,8 @@ stmt_perform : K_PERFORM
new = palloc0(sizeof(PLpgSQL_stmt_perform));
new->cmd_type = PLPGSQL_STMT_PERFORM;
new->lineno = plpgsql_location_to_lineno(@1);
- new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
plpgsql_push_back_token(K_PERFORM);
/*
@@ -943,6 +945,7 @@ stmt_call : K_CALL
new->cmd_type = PLPGSQL_STMT_CALL;
new->lineno = plpgsql_location_to_lineno(@1);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
plpgsql_push_back_token(K_CALL);
new->expr = read_sql_stmt();
new->is_call = true;
@@ -962,6 +965,7 @@ stmt_call : K_CALL
new->cmd_type = PLPGSQL_STMT_CALL;
new->lineno = plpgsql_location_to_lineno(@1);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
plpgsql_push_back_token(K_DO);
new->expr = read_sql_stmt();
new->is_call = false;
@@ -1000,7 +1004,8 @@ stmt_assign : T_DATUM
new = palloc0(sizeof(PLpgSQL_stmt_assign));
new->cmd_type = PLPGSQL_STMT_ASSIGN;
new->lineno = plpgsql_location_to_lineno(@1);
- new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->varno = $1.datum->dno;
/* Push back the head name to include it in the stmt */
plpgsql_push_back_token(T_DATUM);
@@ -1022,6 +1027,7 @@ stmt_getdiag : K_GET getdiag_area_opt K_DIAGNOSTICS getdiag_list ';'
new->cmd_type = PLPGSQL_STMT_GETDIAG;
new->lineno = plpgsql_location_to_lineno(@1);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->is_stacked = $2;
new->diag_items = $4;
@@ -1194,6 +1200,7 @@ stmt_if : K_IF expr_until_then proc_sect stmt_elsifs stmt_else K_END K_IF ';'
new->cmd_type = PLPGSQL_STMT_IF;
new->lineno = plpgsql_location_to_lineno(@1);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->cond = $2;
new->then_body = $3;
new->elsif_list = $4;
@@ -1299,6 +1306,7 @@ stmt_loop : opt_loop_label K_LOOP loop_body
new->cmd_type = PLPGSQL_STMT_LOOP;
new->lineno = plpgsql_location_to_lineno(@2);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+ new->ns = plpgsql_ns_top();
new->label = $1;
new->body = $3.stmts;
@@ -1317,6 +1325,7 @@ stmt_while : opt_loop_label K_WHILE expr_until_loop loop_body
new->cmd_type = PLPGSQL_STMT_WHILE;
new->lineno = plpgsql_location_to_lineno(@2);
new->stmtid = ++plpgsql_curr_compile->nstatements;
+
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Aug 23, 2021 at 1:59 PM Amit Kapila wrote: > > On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com > wrote: > > > > Personally, I also think it will be better to make the behavior consistent. > > Attach the new version patch make both ADD and DROP behave the same as SET > > PUBLICATION > > which refresh all the publications. > > > > I think we can have tests in the separate test file (alter_sub_pub.pl) > like you earlier had in one of the versions. Use some meaningful names > for tables instead of temp1, temp2 as you had in the previous version. > Otherwise, the code changes look good to me. - supported_opts = SUBOPT_REFRESH; - if (isadd) - supported_opts |= SUBOPT_COPY_DATA; + supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA; I think that the currently the doc says copy_data option can be specified except in DROP PUBLICATION case, which needs to be fixed corresponding the above change. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
On Mon, Aug 23, 2021 at 9:53 AM Dilip Kumar wrote: > > On Mon, Aug 23, 2021 at 9:11 AM houzj.f...@fujitsu.com > wrote: > > > 4) > > -extern File SharedFileSetCreate(SharedFileSet *fileset, const char *name); > > -extern File SharedFileSetOpen(SharedFileSet *fileset, const char *name, > > - int mode); > > -extern bool SharedFileSetDelete(SharedFileSet *fileset, const char *name, > > - bool > > error_on_failure); > > extern void SharedFileSetDeleteAll(SharedFileSet *fileset); > > -extern void SharedFileSetUnregister(SharedFileSet *input_fileset); > > > > I noticed the patch delete part of public api, is it better to keep the old > > api and > > let them invoke new api internally ? Having said that, I didn’t find any > > open source > > extension use these old api, so it might be fine to delete them. > > Right, those were internally used by buffile.c but now we have changed > buffile.c to directly use the fileset interfaces, so we better remove > them. > I also don't see any reason to keep those exposed from sharedfileset.h. I see that even in the original commit dc6c4c9dc2, these APIs seem to be introduced to be used by buffile. Andres/Thomas, do let us know if you think otherwise? One more comment: I think v1-0001-Sharedfileset-refactoring doesn't have a way for cleaning up worker.c temporary files on error/exit. It is better to have that to make it an independent patch. -- With Regards, Amit Kapila.
Re: Some leftovers of recent message cleanup?
At Fri, 20 Aug 2021 19:36:02 +0900, Fujii Masao
wrote in
>
>
> On 2021/08/20 11:53, Kyotaro Horiguchi wrote:
> > At Thu, 19 Aug 2021 20:29:42 +0900, Fujii Masao
> > wrote in
> >> On 2021/08/19 17:03, Kyotaro Horiguchi wrote:
> >>> Hello.
> >>> While I was examining message translation for PG14, I found some
> >>> messages that would need to be fixed.
> >>> 0001 is a fix for perhaps-leftovers of the recent message cleanups
> >>> related to "positive integer"(fd90f6ba7a).
> >>
> >> There are still other many messages using "positive" and "negative"
> >> keywords.
> >> We should also fix them at all?
> > I'm not sure, or no if anything. My main point here is not to avoid
> > use of such kind of words, but reducing variations of the effectively
> > the same message from the view of translator burden. The two messages
> > in 0001 are in that category. I noticed those messages accidentally. I
> > don't think they are the only instance of such divergence, but I'm not
> > going to do a comprehensive examination of such divergences.. (Or do I
> > need to check that more comprehensively?)
>
>
> Understood.
>
> - errmsg("modulus for hash partition
> must be a positive
> - integer")));
> + errmsg("modulus for hash partition must be an integer greater than
> zero")));
>
> "an integer greater" should be "an integer value greater" because
> we use that words in other similar log messages like "modulus for
> hash partition must be an integer value greater than zero"
> in src/backend/partitioning/partbounds.c?
Ugh... Of course. I thought I did that way but the actual file is
incorrect... Fixed that in the attached.
> I'm thinking to back-patch this to v11 where hash partitioning
> was supported. On the other hand, the following change should
If I'm not missing something, back to 13, where the "word change" took
place? For 11 and 12, we need to change the *both* messages.
v11, 12
./src/backend/parser/parse_utilcmd.cer")));
Binary file ./src/backend/parser/gram.o matches
./src/backend/partitioning/partbounds.cpg/preproc/ecpg.header
@@ -596,7 +596,7 @@ check_declared_list(const char *name)
{
if (connection)
if (connection && strcmp(ptr->connection, connection)
!= 0)
- mmerror(PARSE_ERROR, ET_WARNING, "connection %s
is overwritten with %s by declare statement %s.", connection, ptr->connection,
name);
+ mmerror(PARSE_ERROR, ET_WARNING, "connection %s
is overwritten with %s by DECLARE statement %s", connection, ptr->connection,
name);
connection = mm_strdup(ptr -> connection);
return true;
}
--
2.27.0
Re: pg_veryfybackup can fail with a valid backup for TLI > 1
On Fri, Aug 20, 2021 at 04:47:15PM +0900, Kyotaro Horiguchi wrote: > Yes, backup_label looks correct. > > backup_label (extract): > START WAL LOCATION: 0/528 (file 00020005) > CHECKPOINT LOCATION: 0/560 > START TIMELINE: 2 Okay. I have worked on that today, did more manual tests, and applied this fix down to 13. The cherry-pick from 14 to 13 only required a s/$master/$primary/ in the test, which was straight-forward. Your patch for 13 did that though: - if (starttli != entry->tli) + if (!XLogRecPtrIsInvalid(entry->begin)) So it would have caused a failure with parent TLIs that have a correct begin location, but we expect the opposite. The patch for 14/HEAD had that right. -- Michael signature.asc Description: PGP signature
Re: Mark all GUC variable as PGDLLIMPORT
On Sun, Aug 22, 2021 at 09:29:01PM +0800, Julien Rouhaud wrote: > On Sun, Aug 22, 2021 at 09:19:42AM -0400, Tom Lane wrote: > > > > Uh, no, it's exactly *not* clear. There are a lot of GUCs that are only > > of interest to particular subsystems. I do not see why being a GUC makes > > something automatically more interesting than any other global variable. > > Usually, the fact that one is global is only so the GUC machinery itself > > can get at it, otherwise it'd be static in the owning module. > > > > As for "extensions should be able to get at the values", the GUC machinery > > already provides uniform mechanisms for doing that safely. Direct access > > to the variable's internal value would be unsafe in many cases. > > Then shouldn't we try to prevent direct access on all platforms rather than > only one? So since the non currently explicitly exported GUC global variables shouldn't be accessible by third-party code, I'm attaching a POC patch that does the opposite of v1: enforce that restriction using a new pg_attribute_hidden() macro, defined with GCC only, to start discussing that topic. It would probably be better to have some other macro (e.g. PG_GLOBAL_PUBLIC and PG_GLOBAL_PRIVATE or similar) to make declarations more consistent, but given the amount of changes it would represent I prefer to have some feedback before spending time on that. >From b43c48a80f985e879a88d9d270fc1e2b283b5732 Mon Sep 17 00:00:00 2001 From: Julien Rouhaud Date: Sun, 22 Aug 2021 15:40:47 +0800 Subject: [PATCH v2] Make all GUC ariables non previously marked as PGDLLIMPORT private. --- src/backend/utils/misc/guc.c | 18 +++--- src/include/access/gin.h | 2 +- src/include/access/tableam.h | 4 +- src/include/access/toast_compression.h| 2 +- src/include/access/xact.h | 12 ++-- src/include/access/xlog.h | 78 +++ src/include/c.h | 7 ++ src/include/catalog/namespace.h | 2 +- src/include/catalog/storage.h | 2 +- src/include/commands/async.h | 2 +- src/include/commands/user.h | 2 +- src/include/commands/vacuum.h | 12 ++-- src/include/fmgr.h| 2 +- src/include/jit/jit.h | 20 +++--- src/include/libpq/auth.h | 4 +- src/include/libpq/libpq.h | 24 +++ src/include/libpq/pqcomm.h| 2 +- src/include/miscadmin.h | 22 +++ src/include/optimizer/geqo.h | 10 +-- src/include/optimizer/optimizer.h | 4 +- src/include/optimizer/planmain.h | 6 +- src/include/parser/parse_expr.h | 2 +- src/include/parser/parser.h | 4 +- src/include/pgstat.h | 6 +- src/include/pgtime.h | 2 +- src/include/postmaster/autovacuum.h | 30 - src/include/postmaster/bgwriter.h | 8 +-- src/include/postmaster/postmaster.h | 28 src/include/postmaster/syslogger.h| 10 +-- src/include/postmaster/walwriter.h| 4 +- src/include/replication/logicallauncher.h | 4 +- src/include/replication/syncrep.h | 2 +- src/include/replication/walreceiver.h | 6 +- src/include/replication/walsender.h | 6 +- src/include/storage/bufmgr.h | 20 +++--- src/include/storage/dsm_impl.h| 4 +- src/include/storage/fd.h | 2 +- src/include/storage/large_object.h| 2 +- src/include/storage/lock.h| 12 ++-- src/include/storage/lwlock.h | 2 +- src/include/storage/pg_shmem.h| 6 +- src/include/storage/predicate.h | 6 +- src/include/storage/proc.h| 2 +- src/include/storage/standby.h | 8 +-- src/include/tcop/tcopprot.h | 6 +- src/include/tsearch/ts_cache.h| 2 +- src/include/utils/array.h | 2 +- src/include/utils/builtins.h | 2 +- src/include/utils/bytea.h | 2 +- src/include/utils/elog.h | 10 +-- src/include/utils/guc.h | 62 +- src/include/utils/pg_locale.h | 8 +-- src/include/utils/plancache.h | 2 +- src/include/utils/ps_status.h | 2 +- src/include/utils/queryjumble.h | 2 +- src/include/utils/rls.h | 2 +- src/include/utils/xml.h | 4 +- 57 files changed, 263 insertions(+), 256 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index a2e0f8de7e..ca2d6042cd 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -129,20 +129,20 @@ #define REALTYPE_PRECISION 17 /* XXX these should appear in other modules' header files */ -extern bool Log_disconnections; -extern int Commit
Re: Proposal: More structured logging
Le vendredi 20 août 2021, 11:31:21 CEST Ronan Dunklau a écrit :
> Le jeudi 19 août 2021, 15:04:30 CEST Alvaro Herrera a écrit :
> > On 2021-Aug-13, Ronan Dunklau wrote:
> > > ereport(NOTICE,
> > >
> > > (errmsg("My log message")),
> > > (errtag("EMITTER", "MYEXTENSION")),
> > > (errtag("MSG-ID", "%d", error_message_id))
> > >
> > > );
> >
> > Interesting idea. I agree this would be useful.
> >
> > > Please find attached a very small POC patch to better demonstrate what I
> > > propose.
> >
> > Seems like a good start. I think a further step towards a committable
> > patch would include a test module that uses the new tagging
> > functionality.
>
> Please find attached the original patch + a new one adding a test module.
> The test module exposes a function for generating logs with tags, and a log
> hook which format the tags in the DETAIL field for easy regression testing.
>
> Next step would be to emit those tags in the CSV logs. I'm not sure what
> representation they should have though: a nested csv in it's own column ? A
> key => value pairs list similar to hstore ? A json object ?
I opted for a JSON representation in the CSV logs, please find attached v3
which is the same thing as v2 with another patch for CSV log output.
>
> Also we should probably make this available to the client too, but once
> again the format of the tag field needs to be defined. Any opinion ?
--
Ronan Dunklau>From e5af5d1a07e65926eee90e0d18443a673d1fcba8 Mon Sep 17 00:00:00 2001
From: Ronan Dunklau
Date: Fri, 13 Aug 2021 15:03:18 +0200
Subject: [PATCH v3 1/3] Add ErrorTag support
---
src/backend/utils/error/elog.c | 48 ++
src/include/utils/elog.h | 10 +++
2 files changed, 58 insertions(+)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index a3e1c59a82..5b9b1b8a72 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -465,6 +465,7 @@ errstart(int elevel, const char *domain)
edata->sqlerrcode = ERRCODE_SUCCESSFUL_COMPLETION;
/* errno is saved here so that error parameter eval can't change it */
edata->saved_errno = errno;
+ edata->tags = NIL;
/*
* Any allocations for this error state level should go into ErrorContext
@@ -516,6 +517,7 @@ errfinish(const char *filename, int lineno, const char *funcname)
int elevel;
MemoryContext oldcontext;
ErrorContextCallback *econtext;
+ ListCell *lc;
recursion_depth++;
CHECK_STACK_DEPTH();
@@ -621,7 +623,18 @@ errfinish(const char *filename, int lineno, const char *funcname)
pfree(edata->constraint_name);
if (edata->internalquery)
pfree(edata->internalquery);
+ /* Every tag should have been palloc'ed */
+ if (edata->tags != NIL)
+ {
+ foreach(lc, edata->tags)
+ {
+ ErrorTag *tag = (ErrorTag *) lfirst(lc);
+ pfree(tag->tagvalue);
+ pfree(tag);
+ }
+ pfree(edata->tags);
+ }
errordata_stack_depth--;
/* Exit error-handling context */
@@ -1192,6 +1205,41 @@ errhint_plural(const char *fmt_singular, const char *fmt_plural,
return 0; /* return value does not matter */
}
+int
+errtag(const char *tag, const char *fmt_value,...)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+ ErrorTag *etag;
+ MemoryContext oldcontext;
+ StringInfoData buf;
+
+ recursion_depth++;
+ CHECK_STACK_DEPTH();
+ oldcontext = MemoryContextSwitchTo(edata->assoc_context);
+ etag = palloc(sizeof(ErrorTag));
+ etag->tagname = tag;
+ initStringInfo(&buf);
+ for (;;)
+ {
+ va_list args;
+ int needed;
+
+ errno = edata->saved_errno;
+ va_start(args, fmt_value);
+ needed = appendStringInfoVA(&buf, fmt_value, args);
+ va_end(args);
+ if (needed == 0)
+ break;
+ enlargeStringInfo(&buf, needed);
+ }
+ etag->tagvalue = pstrdup(buf.data);
+ edata->tags = lappend(edata->tags, etag);
+ pfree(buf.data);
+ MemoryContextSwitchTo(oldcontext);
+ recursion_depth--;
+ return 0;
+}
+
/*
* errcontext_msg --- add a context error message text to the current error
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index f53607e12e..1c490d1b11 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -15,6 +15,7 @@
#define ELOG_H
#include
+#include "nodes/pg_list.h"
/* Error level codes */
#define DEBUG5 10 /* Debugging messages, in categories of
@@ -193,6 +194,8 @@ extern int errhint(const char *fmt,...) pg_attribute_printf(1, 2);
extern int errhint_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4);
+extern int errtag(const char *tag, const char *fmt_value,...) pg_attribute_printf(2, 3);
+
/*
* errcontext() is typically called in error context callback functions, not
* within an ereport() invocation. The callback function can be in a different
@@ -395,11 +398,18 @@ typedef struct ErrorData
int internalpos; /* cursor index into internalquery */
char *internalquery; /* text of internally-ge
