Re: remove more archiving overhead
On Sat, Sep 17, 2022 at 02:54:27PM -0700, Nathan Bossart wrote: > On Sat, Sep 17, 2022 at 11:46:39AM +0200, Peter Eisentraut wrote: > >> > --- a/doc/src/sgml/backup.sgml > >> > +++ b/doc/src/sgml/backup.sgml > >> > @@ -691,11 +691,9 @@ test ! -f > >> > /mnt/server/archivedir/000100A90065 && cp pg_wal/0 > >> > system crashes before the server makes a durable record of > >> > archival success, > >> > the server will attempt to archive the file again after restarting > >> > (provided > >> > archiving is still enabled). When an archive library encounters a > >> > -pre-existing file, it may return true if the WAL > >> > file has > >> > +pre-existing file, it should return true if the > >> > WAL file has > >> > identical contents to the pre-existing archive and the > >> > pre-existing archive > >> > -is fully persisted to storage. Alternatively, the archive library > >> > may > >> > -return false anytime a pre-existing file is > >> > encountered, > >> > -but this will require manual action by an administrator to resolve. > >> > If a > >> > +is fully persisted to storage. If a > >> > pre-existing file contains different contents than the WAL file > >> > being > >> > archived, the archive library must return > >> > false. > >> > >> Works for me. Thanks. > > > > This documentation change only covers archive_library. How are users of > > archive_command supposed to handle this? > > I believe users of archive_command need to do something similar to what is > described here. However, it might be more reasonable to expect > archive_command users to simply return false when there is a pre-existing > file, as the deleted text notes. IIRC that is why I added that sentence > originally. What makes the answer for archive_command diverge from the answer for archive_library?
Re: is_superuser is not documented
On 2022/09/14 14:27, bt22kawamotok wrote:
I update patch to reflect master update.
Thanks for updating the patch!
+
+Shows whether the current user is a superuser or not.
+
How about adding the note about when this parameter can change,
like we do for in_hot_standby docs? I applied this change to the patch.
Attached is the updated version of the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 700914684d..0f910d580c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -10584,6 +10584,23 @@ dynamic_library_path =
'C:\tools\postgresql;H:\my_project\lib;$libdir'
+
+ is_superuser (boolean)
+
+ is_superuser configuration
parameter
+
+
+
+
+Reports whether the current user is a superuser.
+Within a session, this can change when the current user is changed by
+SET ROLE,
+
+SET SESSION AUTHORIZATION, etc.
+
+
+
+
lc_collate (string)
diff --git a/doc/src/sgml/ref/show.sgml b/doc/src/sgml/ref/show.sgml
index b3747b119f..d8721be805 100644
--- a/doc/src/sgml/ref/show.sgml
+++ b/doc/src/sgml/ref/show.sgml
@@ -100,15 +100,6 @@ SHOW ALL
-
-
-IS_SUPERUSER
-
-
- True if the current role has superuser privileges.
-
-
-
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 550e95056c..1e57ee16b8 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -989,11 +989,10 @@ struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- /* Not for general use --- used by SET SESSION AUTHORIZATION */
- {"is_superuser", PGC_INTERNAL, UNGROUPED,
+ {"is_superuser", PGC_INTERNAL, PRESET_OPTIONS,
gettext_noop("Shows whether the current user is a
superuser."),
NULL,
- GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL |
GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
},
&session_auth_is_superuser,
false,
Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)
On Sun, Sep 18, 2022 at 7:38 AM Fujii Masao wrote: > > On 2022/09/17 16:18, Bharath Rupireddy wrote: > > Good idea. It makes a lot more sense to me, because xlog.c is already > > a file of 9000 LOC. I've created xlogbackup.c/.h files and added the > > new code there. Once this patch gets in, I can offer my hand to move > > do_pg_backup_start() and do_pg_backup_stop() from xlog.c and if okay, > > pg_backup_start() and pg_backup_stop() from xlogfuncs.c to > > xlogbackup.c/.h. Then, we might have to create new get/set APIs for > > XLogCtl fields that do_pg_backup_start() and do_pg_backup_stop() > > access. > > The definition of SessionBackupState enum type also should be in xlogbackup.h? Correct. Basically, all the backup related code from xlog.c, xlogfuncs.c and elsewhere can go to xlogbackup.c/.h. I will focus on that refactoring patch once this gets in. > > We need to carry tablespace_map contents from do_pg_backup_start() > > till pg_backup_stop(), backup_label and history_file too are easy to > > carry across. Hence it will be good to have all of them i.e. > > tablespace_map, backup_label and history_file in the BackupState > > structure. IMO, this way is good. > > backup_label and history_file are not carried between pg_backup_start() > and _stop(), so don't need to be saved in BackupState. Their contents > can be easily created from other saved fields in BackupState, > if necessary. So at least for me it's better to get rid of them from > BackupState and don't allocate TopMemoryContext memory for them. Yeah, but they have to be carried from do_pg_backup_stop() to pg_backup_stop() or callers and also instead of keeping tablespace_map in BackupState and others elsewhere don't seem to be a good idea to me. IMO, BackupState is a good place to contain all the information that's carried across various functions. I've changed the code to lazily (upon first use in the backend) allocate memory for all of them as we're concerned of the memory allocation beforehand. > > Yeah, I think that can be solved by passing in backup_state to > > do_pg_abort_backup(). If okay, I can work on this too as 0002 patch in > > this thread or we can discuss this separately to get more attention > > after this refactoring patch gets in. > > Or, to avoid such memory bloat, how about allocating the memory for > backup_state only when it's NULL? Ah my bad, I missed that. Done now. > > I'm attaching v5 patch with the above review comments addressed, > > please review it further. > > Thanks for updating the patch! Thanks for reviewing it. > + charstartxlogfile[MAXFNAMELEN_BACKUP]; /* backup start > WAL file */ > > + charstopxlogfile[MAXFNAMELEN_BACKUP]; /* backup > stop WAL file */ > > These file names seem not necessary in BackupState because they can be > calculated from other fields like startpoint and starttli, etc when > making backup_label and history file contents. If we remove them from > BackupState, we can also remove the definition of MAXFNAMELEN_BACKUP > macro from xlogbackup.h. Done. > + /* construct backup_label contents */ > + build_backup_content(state, false); > > In basebackup case, build_backup_content() is called unnecessary twice > because do_pg_stop_backup() and its caller, perform_base_backup() call > that. This makes me think that it's better to get rid of the call to > build_backup_content() from do_pg_backup_stop(). Instead its callers, > perform_base_backup() and pg_backup_stop() should call that. Yeah, it's a good idea. Done that way. It's easier because we can create backup_label file contents at any point of time after pg_backup_start(). -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com v6-0001-Refactor-backup-related-code.patch Description: Binary data
Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)
On Sun, Sep 18, 2022 at 1:23 PM Bharath Rupireddy wrote: cfbot fails [1] with v6 patch. I made a silly mistake by not checking the output of "make check-world -j 16" fully, I just saw the end message "All tests successful." before posting the v6 patch. The failure is due to perform_base_backup() accessing BackupState's tablespace_map without a null check, so I fixed it. Sorry for the noise. Please review the attached v7 patch further. [1] https://cirrus-ci.com/task/5816966114967552?logs=test_world#L720 -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com v7-0001-Refactor-backup-related-code.patch Description: Binary data
Re: missing indexes in indexlist with partitioned tables
On Sun, 18 Sept 2022 at 07:00, Tom Lane wrote: > > Alvaro Herrera writes: > > After a bit of trawling through the archives, I found it here: > > https://www.postgresql.org/message-id/20180124162006.pmapfiznhgngwtjf%40alvherre.pgsql > > I think there was insufficient discussion and you're probably right that > > it wasn't the best fix. I don't object to finding another fix. > > FWIW, I don't see any big problem with what you did. We'd need to > do something more like what David suggests if the planner ever has > a reason to consider partitioned indexes. But as long as it does > not, why expend the time to build data structures representing them? Did you miss the report about left join removals not working with partitioned tables due to lack of unique proofs? That seems like a good enough reason to me. > And we'd have to add code in quite a few places to ignore them, > once they're in indexlist. I think the same is true for "hypothetical" indexes. Maybe that would be a good field to grep on to find the places that need to be addressed. David
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
I wrote:
> Attached is an incomplete POC patch that suppresses these warnings
> in nodeFuncs.c itself and in costsize.c, which I selected at random
> as a typical caller. I'll push forward with converting the other
> call sites if this way seems good to people.
Here's a fleshed-out patch that gets rid of all warnings of this sort
(tested on clang version 15.0.0).
While I remain happy enough with what has to be done in nodeFuncs.c,
I'm really not happy at all with this point:
> It's sad to note that this exercise in hoop-jumping actually leaves
> us with net LESS type safety, because the outside callers of
> cost_qual_eval_walker are no longer constrained to call it with
> the appropriate kind of context struct. Thanks, C committee.
There are a lot of these walker/mutator functions and hence a whole
lot of opportunity to pass the wrong thing, not only from the outer
non-recursive call points but during internal recursions in the
walkers/mutators themselves.
I think we ought to seriously consider the alternative of changing
nodeFuncs.c about like I have here, but not touching the walkers/mutators,
and silencing the resulting complaints about function type casting by
doing the equivalent of
-return expression_tree_walker(node, cost_qual_eval_walker,
- (void *) context);
+return expression_tree_walker(node,
+ (tree_walker_callback) cost_qual_eval_walker,
+ (void *) context);
We could avoid touching all the call sites by turning
expression_tree_walker and friends into macro wrappers that incorporate
these casts. This is fairly annoying, in that it gives up the function
type safety the C committee wants to impose on us; but I really think
the data type safety that we're giving up in this version of the patch
is a worse hazard.
BTW, I was distressed to discover that someone decided they could
use ExecShutdownNode as a planstate_tree_walker() walker even though
its argument list is not even the right length. I'm a bit flabbergasted
that we seem to have gotten away with that so far, because I'd have
thought for sure that it'd break some platform's convention for which
argument gets passed where. I think we need to fix that, independently
of what we do about the larger scope of these problems. To avoid an
API break, I propose making ExecShutdownNode just be a one-liner that
calls an internal ExecShutdownNode_walker() function. (I've not done
it that way in the attached, though.)
Thoughts?
regards, tom lane
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 39768fa22b..f127f20fd8 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -206,8 +206,7 @@ static void reportDependentObjects(const ObjectAddresses *targetObjects,
static void deleteOneObject(const ObjectAddress *object,
Relation *depRel, int32 flags);
static void doDeletion(const ObjectAddress *object, int flags);
-static bool find_expr_references_walker(Node *node,
- find_expr_references_context *context);
+static bool find_expr_references_walker(Node *node, void *ctx);
static void process_function_rte_ref(RangeTblEntry *rte, AttrNumber attnum,
find_expr_references_context *context);
static void eliminate_duplicate_dependencies(ObjectAddresses *addrs);
@@ -1738,9 +1737,10 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
* the collation is being freshly introduced to the expression.
*/
static bool
-find_expr_references_walker(Node *node,
- find_expr_references_context *context)
+find_expr_references_walker(Node *node, void *ctx)
{
+ find_expr_references_context *context = ctx;
+
if (node == NULL)
return false;
if (IsA(node, Var))
@@ -2283,7 +2283,7 @@ find_expr_references_walker(Node *node,
context->rtables = lcons(query->rtable, context->rtables);
result = query_tree_walker(query,
find_expr_references_walker,
- (void *) context,
+ ctx,
QTW_IGNORE_JOINALIASES |
QTW_EXAMINE_SORTGROUP);
context->rtables = list_delete_first(context->rtables);
@@ -2352,8 +2352,7 @@ find_expr_references_walker(Node *node,
/* fall through to examine arguments */
}
- return expression_tree_walker(node, find_expr_references_walker,
- (void *) context);
+ return expression_tree_walker(node, find_expr_references_walker, ctx);
}
/*
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 053d2ca5ae..671a41d3b6 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -63,7 +63,7 @@ static void ExplainPrintJIT(ExplainState *es, int jit_flags,
static void report_triggers(ResultRelInfo *rInfo, bool show_relname,
ExplainState *es);
static double elapsed_time(instr_time *starttime);
-static bool ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used);
+static
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
On Mon, Sep 19, 2022 at 8:57 AM Tom Lane wrote: > I think we ought to seriously consider the alternative of changing > nodeFuncs.c about like I have here, but not touching the walkers/mutators, > and silencing the resulting complaints about function type casting by > doing the equivalent of > > -return expression_tree_walker(node, cost_qual_eval_walker, > - (void *) context); > +return expression_tree_walker(node, > + (tree_walker_callback) > cost_qual_eval_walker, > + (void *) context); > > We could avoid touching all the call sites by turning > expression_tree_walker and friends into macro wrappers that incorporate > these casts. This is fairly annoying, in that it gives up the function > type safety the C committee wants to impose on us; but I really think > the data type safety that we're giving up in this version of the patch > is a worse hazard. But is it defined behaviour? https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type > BTW, I was distressed to discover that someone decided they could > use ExecShutdownNode as a planstate_tree_walker() walker even though > its argument list is not even the right length. I'm a bit flabbergasted > that we seem to have gotten away with that so far, because I'd have > thought for sure that it'd break some platform's convention for which > argument gets passed where. I think we need to fix that, independently > of what we do about the larger scope of these problems. To avoid an > API break, I propose making ExecShutdownNode just be a one-liner that > calls an internal ExecShutdownNode_walker() function. (I've not done > it that way in the attached, though.) Huh... wouldn't systems that pass arguments right-to-left on the stack receive NULL for node? That'd include the SysV i386 convention used on Linux, *BSD etc. But that can't be right or we'd know about it... But certainly +1 for fixing that regardless.
Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica.
Hello!
Thank you very much for your feedback and essential remarks.
On 07.09.2022 10:39, Kyotaro Horiguchi wrote:
It lets XLogPageRead run the same check with what CreateRestartPoint
does, so it basically works (it is forgetting a lock, though. The
reason for omitting the lock in CreateRestartPoint is that it knows
checkopinter is the only updator of the shared variable.). I'm not
sure I like that for the code duplication.
I'm not sure we need to fix that but if we do that, I would impletent
IsNewCheckPointWALRecs() using XLogCtl->RedoRecPtr and
XLogCtl->lastCheckPoint.redo instead since they are protected by the
same lock, and they work more correct way, that is, that can avoid
restartpoint requests while the last checkpoint is running. And I
would rename it as RestartPointAvailable() or something like that.
Corrected patch is attached (v2-0001-Fix-burst-checkpoint_req-growth.patch).
The access to Controlfile was removed so lwlock seems to be not needed.
Some logic duplication is still present and and i'm not quite sure if
it's possible to get rid of it. Would be glad to any suggestions.
Or I might want to add XLogRestartpointNeeded(readSegNo) to reduce the
required number of info_lck by reading XLogCtl members at once.
If we place this check into the XLogCheckpointNeeded() this will lead to a
double
take of info_lck in XLogPageRead() when the restartpoint request is forming.
As it's done now taking of info_lck will be more rarely.
It seems i probably didn't understand your idea, please clarify it for me.
Depends on how we see the counter value. I think this can be annoying
but not a bug. CreateRestartPoint already handles that case.
Yes! It is in fact annoying as docs says that checkpoint_req counts
"the number of requested checkpoints that have been performed".
But really checkpoints_req counts both the number of checkpoints requests
and restartpoint ones which may not be performed and resources are not spent.
The second frightening factor is the several times faster growth
of the checkpoints_timed counter on the replica vs primary due to scheduling
replays in 15 second if an attempt to create the restartpoint failed.
Here is a patch that leaves all logic as is, but adds a stats about
restartpoints. (v1-0001-Add-restartpoint-stats.patch)
.
For instance, for the same period on primary with this patch:
# SELECT CURRENT_TIME; select * from pg_stat_bgwriter \gx
current_time
00:19:15.794561+03
(1 row)
-[ RECORD 1 ]-+-
checkpoints_timed | 4
checkpoints_req | 10
restartpoints_timed | 0
restartpoints_req | 0
restartpoints_done| 0
On replica:
# SELECT CURRENT_TIME; select * from pg_stat_bgwriter \gx
current_time
00:19:11.363009+03
(1 row)
-[ RECORD 1 ]-+--
checkpoints_timed | 0
checkpoints_req | 0
restartpoints_timed | 42
restartpoints_req | 67
restartpoints_done| 10
Only the counters checkpoints_timed, checkpoints_req and restartpoints_done give
the indication of resource-intensive operations.
Without this patch, the user would see on the replica something like this:
checkpoints_timed | 42
checkpoints_req | 109
With best regards,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companycommit d5a58d8585692be0d24db9414859088e3e7f7dad
Author: Anton A. Melnikov
Date: Tue Sep 6 12:18:56 2022 +0300
Remove burst growth of the checkpoint_req on replica.
with correcttions according to comment:
https://www.postgresql.org/message-id/20220907.163946.2214338139097492905.horikyota.ntt%40gmail.com
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 81d339d57d..3ed1a87943 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9054,3 +9054,20 @@ SetWalWriterSleeping(bool sleeping)
XLogCtl->WalWriterSleeping = sleeping;
SpinLockRelease(&XLogCtl->info_lck);
}
+
+/*
+ * Check if a new checkpoint WAL record has been received since the
+ * last restartpoint. So it is possible to create a new one.
+ */
+bool RestartPointAvailable(void)
+{
+ bool result = false;
+
+ SpinLockAcquire(&XLogCtl->info_lck);
+ if (!XLogRecPtrIsInvalid(XLogCtl->lastCheckPointRecPtr)
+ && XLogCtl->lastCheckPoint.redo > XLogCtl->RedoRecPtr)
+result = true;
+ SpinLockRelease(&XLogCtl->info_lck);
+
+ return result;
+}
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index b41e682664..7236e0f0a4 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -3199,7 +3199,15 @@ XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
{
(void) GetRedoRecPtr();
if (XLogCheckpointNeeded(readSegNo))
- RequestCheckpoint(CHECKPOINT_CAUSE_XLOG);
+{
+ /*
+ * If there is no new checkp
Re: Pruning never visible changes
On Fri, 16 Sept 2022 at 10:27, Tom Lane wrote: > > Simon Riggs writes: > > A user asked me whether we prune never visible changes, such as > > BEGIN; > > INSERT... > > UPDATE.. (same row) > > COMMIT; > > Didn't we just have this discussion in another thread? Well. not "just" :) commit 44e4bbf75d56e643b6afefd5cdcffccb68cce414 Author: Tom Lane Date: Fri Apr 29 16:29:42 2011 -0400 Remove special case for xmin == xmax in HeapTupleSatisfiesVacuum(). VACUUM was willing to remove a committed-dead tuple immediately if it was deleted by the same transaction that inserted it. The idea is that such a tuple could never have been visible to any other transaction, so we don't need to keep it around to satisfy MVCC snapshots. However, there was already an exception for tuples that are part of an update chain, and this exception created a problem: we might remove TOAST tuples (which are never part of an update chain) while their parent tuple stayed around (if it was part of an update chain). This didn't pose a problem for most things, since the parent tuple is indeed dead: no snapshot will ever consider it visible. But MVCC-safe CLUSTER had a problem, since it will try to copy RECENTLY_DEAD tuples to the new table. It then has to copy their TOAST data too, and would fail if VACUUM had already removed the toast tuples. Easiest fix is to get rid of the special case for xmin == xmax. This may delay reclaiming dead space for a little bit in some cases, but it's by far the most reliable way to fix the issue. Per bug #5998 from Mark Reid. Back-patch to 8.3, which is the oldest version with MVCC-safe CLUSTER.
Re: Making C function declaration parameter names consistent with corresponding definition names
On Sun, 18 Sept 2022 at 07:59, Peter Geoghegan wrote: > Attached revision adds a new, third patch. This fixes all the warnings > from clang-tidy's "readability-named-parameter" check. The extent of > the code churn seems acceptable to me. +1 to the idea of aligning the parameter names between function declarations and definitions. I had a look at the v2-0001 patch and noted down a few things while reading: 1. In getJsonPathVariable you seem to have mistakenly removed a parameter from the declaration. 2. You changed the name of the parameter in the definition of ScanCKeywordLookup(). Is it not better to keep the existing name there so that that function is consistent with ScanKeywordLookup()? 3. Why did you rename the parameter in the definition of nocachegetattr()? Wouldn't it be better just to rename in the declaration. To me, "tup" does not really seem better than "tuple" here. 4. In the definition of ExecIncrementalSortInitializeWorker() you've renamed pwcxt to pcxt, but it seems that the other *InitializeWorker() functions call this pwcxt. Is it better to keep those consistent? I understand that you've done this for consistency with *InitializeDSM() and *Estimate() functions, but I'd rather see it remain consistent with the other *InitializeWorker() functions instead. (I'd not be against a wider rename so all those functions use the same name.) 5. In md.c I see you've renamed a few "forkNum" variables to "formnum". Maybe it's worth also doing the same in mdexists(). mdcreate() is also external and got the rename, so I'm not quite sure why mdexists() would be left. David
Re: Making C function declaration parameter names consistent with corresponding definition names
On Sun, Sep 18, 2022 at 4:38 PM David Rowley wrote: > 1. In getJsonPathVariable you seem to have mistakenly removed a > parameter from the declaration. That was left behind following a recent rebase. Will fix. Every other issue you've raised is some variant of: "I see that you've made a subjective decision to resolve this particular inconsistency on the declaration side by following this particular approach. Why did you do it that way?" This is perfectly reasonable, and it's possible that I made clear mistakes in some individual cases. But overall it's not surprising that somebody else wouldn't handle it in exactly the same way. There is no question that some of these decisions are a little arbitrary. > 2. You changed the name of the parameter in the definition of > ScanCKeywordLookup(). Is it not better to keep the existing name there > so that that function is consistent with ScanKeywordLookup()? Because it somehow felt slightly preferable than introducing a .h level inconsistency between ScanECPGKeywordLookup() and ScanCKeywordLookup(). This is about as hard to justify as justifying why one prefers a slightly different shade of beige when comparing two pages from a book of wallpaper samples. > 3. Why did you rename the parameter in the definition of > nocachegetattr()? Wouldn't it be better just to rename in the > declaration. To me, "tup" does not really seem better than "tuple" > here. Again, greater consistency at the .h level won out here. Granted it's still not perfectly consistent, since I didn't take that to its logical conclusion and make sure that the .h file was consistent, because then we'd be talking about why I did that. :-) > 4. In the definition of ExecIncrementalSortInitializeWorker() you've > renamed pwcxt to pcxt, but it seems that the other *InitializeWorker() > functions call this pwcxt. Is it better to keep those consistent? I > understand that you've done this for consistency with *InitializeDSM() > and *Estimate() functions, but I'd rather see it remain consistent > with the other *InitializeWorker() functions instead. (I'd not be > against a wider rename so all those functions use the same name.) Again, I was looking at this at the level of the .h file (in this case nodeIncrementalSort.h). It never occurred to me to consider other *InitializeWorker() functions. Offhand I think that we should change all of the other *InitializeWorker() functions. I think that things got like this because somebody randomly made one of them pwcxt at some point, which was copied later on. > 5. In md.c I see you've renamed a few "forkNum" variables to > "formnum". Maybe it's worth also doing the same in mdexists(). > mdcreate() is also external and got the rename, so I'm not quite sure > why mdexists() would be left. Yeah, I think that we might as well be perfectly consistent. Making automated refactoring tools work better here is of course a goal of mine -- which is especially useful for making everything consistent at the whole-interface (or header file) level. I wasn't sure how much of that to do up front vs in a later commit. -- Peter Geoghegan
Re: [RFC] building postgres with meson - v13
On 15.09.22 04:26, Andres Freund wrote: Attached is v13 of the meson patchset. The biggest changes are: Did something about warning flags change from the previous patch set? I see it's building with -Wextra now, which combined with -Werror causes the build to fail for me. I have never encountered that with any of the previous patch sets.
Re: Making C function declaration parameter names consistent with corresponding definition names
On Sun, Sep 18, 2022 at 5:08 PM Peter Geoghegan wrote: > Again, I was looking at this at the level of the .h file (in this case > nodeIncrementalSort.h). It never occurred to me to consider other > *InitializeWorker() functions. > > Offhand I think that we should change all of the other > *InitializeWorker() functions. I think that things got like this > because somebody randomly made one of them pwcxt at some point, which > was copied later on. On second thought I definitely got this wrong (it's not subjective after all). I didn't notice that there are actually 2 different datatypes involved here, justifying a different naming convention for each. In other words, the problem really was in the .h file, not in the .c file, so I should simply fix the declaration of ExecIncrementalSortInitializeWorker() and call it a day. There is no reason why ExecIncrementalSortInitializeWorker() ought to be consistent with other functions that appear in the same header file, since (if you squint) you'll notice that the data types are also different. -- Peter Geoghegan
Re: [RFC] building postgres with meson - v13
Hi, On September 18, 2022 5:24:06 PM PDT, Peter Eisentraut wrote: >On 15.09.22 04:26, Andres Freund wrote: >> Attached is v13 of the meson patchset. The biggest changes are: > >Did something about warning flags change from the previous patch set? I see >it's building with -Wextra now, which combined with -Werror causes the build >to fail for me. I have never encountered that with any of the previous patch >sets. In older versions of the patch the default warning level was set to include Wextra, and I had added my local flags to suppress uninteresting warnings. Comparing the warning flags I reduced the warning level and removed the suppressing flags - but changing default options only affects new build trees. To change existing ones do meson configure -Dwarning_level=1 -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Add common function ReplicationOriginName.
Hi. There are already multiple places that are building the subscription origin name, and there are more coming with some new patches [1] doing the same: e.g. snprintf(originname, sizeof(originname), "pg_%u", subid); ~~ IMO it is better to encapsulate this name formatting in common code instead of the format string being scattered around the place. PSA a patch to add a common function ReplicationOriginName. This is the equivalent of a similar function for tablesync (ReplicationOriginNameForTablesync) which already existed. -- [1] https://www.postgresql.org/message-id/flat/CAA4eK1%2BwyN6zpaHUkCLorEWNx75MG0xhMwcFhvjqm2KURZEAGw%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia v1-0001-Add-common-function-ReplicationOriginName.patch Description: Binary data
Fix typos in code comments
Hi, While working on some other patches, I found serval typos(duplicate words and incorrect function name reference) in the code comments. Here is a small patch to fix them. Best regards, Hou zhijie 0001-fix-typos.patch Description: 0001-fix-typos.patch
Re: Fix typos in code comments
On Mon, Sep 19, 2022 at 8:14 AM houzj.f...@fujitsu.com wrote: > > While working on some other patches, I found serval typos(duplicate words and > incorrect function name reference) in the code comments. Here is a small patch > to fix them. > Thanks, the patch looks good to me. I'll push this. -- With Regards, Amit Kapila.
Re: Fix typos in code comments
Hi On Sep 19, 2022, 10:57 +0800, Amit Kapila , wrote: > On Mon, Sep 19, 2022 at 8:14 AM houzj.f...@fujitsu.com > wrote: > > > > While working on some other patches, I found serval typos(duplicate words > > and > > incorrect function name reference) in the code comments. Here is a small > > patch > > to fix them. > > > > Thanks, the patch looks good to me. I'll push this. > > -- > With Regards, > Amit Kapila. > > Good catch. There is a similar typo in doc, runtime.sgml. ```using TLS protocols enabled by by setting the parameter```
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
Thomas Munro writes: > On Mon, Sep 19, 2022 at 8:57 AM Tom Lane wrote: >> ... This is fairly annoying, in that it gives up the function >> type safety the C committee wants to impose on us; but I really think >> the data type safety that we're giving up in this version of the patch >> is a worse hazard. > But is it defined behaviour? > https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type Well, what we're talking about is substituting "void *" (which is required to be compatible with "char *") for a struct pointer type. Standards legalese aside, that could only be a problem if the platform ABI handles "char *" differently from struct pointer types. The last architecture I can remember dealing with where that might actually be a thing was the PDP-10. Everybody has learned better since then, but the C committee is apparently still intent on making the world safe for crappy machine architectures. Also, if you want to argue that "void *" is not compatible with struct pointer types, then it's not real clear to me that we aren't full of other spec violations, because we sure do a lot of casting across that (and even more with this patch as it stands). I don't have the slightest hesitation about saying that if there's still an architecture out there that's like that, we won't support it. I also note that our existing code in this area would break pretty thoroughly on such a machine, so this isn't making it worse. regards, tom lane
Re: Making C function declaration parameter names consistent with corresponding definition names
On Mon, 19 Sept 2022 at 15:04, Peter Geoghegan wrote: > The general structure of the patchset is now a little more worked out. > Although it's still not close to being commitable, it should give you > a better idea of the kind of structure that I'm aiming for. I think > that this should be broken into a few different parts based on the > area of the codebase affected (not the type of check used). Even that > aspect needs more work, because there is still one massive patch -- > this is now the sixth and final patch. Thanks for updating the patches. I'm slightly confused about "still not close to being commitable" along with "this is now the sixth and final patch.". That seems to imply that you're not planning to send any more patches but you don't think this is commitable. I'm assuming I've misunderstood that. I don't have any problems with 0001, 0002 or 0003. Looking at 0004 I see a few issues: 1. ConnectDatabase() seems to need a bit more work in the header comment. There's a reference to AH and AHX. The parameter is now called "A". 2. setup_connection() still references AH->use_role in the comments (line 1102). Similar problem on line 1207 with AH->sync_snapshot_id 3. setupDumpWorker() still makes references to AH->sync_snapshot_id and AH->use_role in the comments. The parameter is now called "A". 4. dumpSearchPath() still has a comment which references AH->searchpath 0005 looks fine. I've not looked at 0006 again. David
Re: Making C function declaration parameter names consistent with corresponding definition names
On Sun, Sep 18, 2022 at 9:07 PM David Rowley wrote: > I'm slightly confused about "still not close to being commitable" > along with "this is now the sixth and final patch.". That seems to > imply that you're not planning to send any more patches but you don't > think this is commitable. I'm assuming I've misunderstood that. I meant that the "big patch" now has a new order -- it is sixth/last in the newly revised patchset, v3. I don't know how many more patch revisions will be required, but at least one or two more revisions seem likely. Here is the stuff that it less ready, or at least seems ambiguous: 1. The pg_dump patch is relatively opinionated about how to resolve inconsistencies, and makes quite a few changes to the .c side. Seeking out the "lesser inconsistency" resulted in more lines being changed. Maybe you won't see it the same way (maybe you'll prefer the other trade-off). That's just how it ended up. 2. The same thing is true to a much smaller degree with the jsonb patch. 3. The big patch itself is...well, very big. And written on autopilot, to a certain degree. So that one just needs more careful examination, on general principle. > Looking at 0004 I see a few issues: > > 1. ConnectDatabase() seems to need a bit more work in the header > comment. There's a reference to AH and AHX. The parameter is now > called "A". > > 2. setup_connection() still references AH->use_role in the comments > (line 1102). Similar problem on line 1207 with AH->sync_snapshot_id > > 3. setupDumpWorker() still makes references to AH->sync_snapshot_id > and AH->use_role in the comments. The parameter is now called "A". > > 4. dumpSearchPath() still has a comment which references AH->searchpath Will fix all those in the next revision. Thanks. -- Peter Geoghegan
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
On Mon, Sep 19, 2022 at 3:39 PM Tom Lane wrote: > Thomas Munro writes: > > On Mon, Sep 19, 2022 at 8:57 AM Tom Lane wrote: > >> ... This is fairly annoying, in that it gives up the function > >> type safety the C committee wants to impose on us; but I really think > >> the data type safety that we're giving up in this version of the patch > >> is a worse hazard. > > > But is it defined behaviour? > > https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type > > Well, what we're talking about is substituting "void *" (which is > required to be compatible with "char *") for a struct pointer type. > Standards legalese aside, that could only be a problem if the platform > ABI handles "char *" differently from struct pointer types. The last > architecture I can remember dealing with where that might actually be > a thing was the PDP-10. Everybody has learned better since then, but > the C committee is apparently still intent on making the world safe > for crappy machine architectures. > > Also, if you want to argue that "void *" is not compatible with struct > pointer types, then it's not real clear to me that we aren't full of > other spec violations, because we sure do a lot of casting across that > (and even more with this patch as it stands). > > I don't have the slightest hesitation about saying that if there's > still an architecture out there that's like that, we won't support it. > I also note that our existing code in this area would break pretty > thoroughly on such a machine, so this isn't making it worse. Yeah, I don't expect it to be a practical problem on any real system (that is, I don't expect any real calling convention to transfer a struct T * argument in a different place than void *). I just wanted to mention that it's a new liberty. It's one thing to cast struct T * to void * and back before dereferencing, and another to cast a pointer to a function that takes struct T * to a pointer to a function that takes void * and call it. I considered proposing that myself when first reporting this problem, but fear of language lawyers put me off.
Re: Typo in xact.c
On Fri, Sep 16, 2022 at 10:51 AM John Naylor wrote: > > On Fri, Sep 16, 2022 at 10:11 AM Kyotaro Horiguchi > wrote: > > > > The patch seems to me covering all occurances of PG_PROC as PGPROC. > > +1 since this hinders grep-ability. Pushed this. > > I found several uses of PG_PROC as (pg_catalog.)pg_proc, which is > > quite confusing, too.. > > It's pretty obvious to me what that refers to in primnodes.h, although > the capitalization of (some, but not all) catalog names in comments in > that file is a bit strange. Maybe not worth changing there. I left this alone. It's not wrong, and I don't think it's confusing in context. -- John Naylor EDB: http://www.enterprisedb.com
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
Thomas Munro writes: > On Mon, Sep 19, 2022 at 3:39 PM Tom Lane wrote: >> I also note that our existing code in this area would break pretty >> thoroughly on such a machine, so this isn't making it worse. > Yeah, I don't expect it to be a practical problem on any real system > (that is, I don't expect any real calling convention to transfer a > struct T * argument in a different place than void *). I just wanted > to mention that it's a new liberty. No, it's not, because the existing coding here is already assuming that. The walker callbacks are generally declared as taking a "struct *" second parameter, but expression_tree_walker et al think they are passing a "void *" to them. Even if a platform ABI had some weird special rule about how to call functions that you don't know the argument list for, it wouldn't fix this because the walkers sure do know what their arguments are. The only reason this code works today is that in practice, "void *" *is* ABI-compatible with "struct *". I'm not excited about creating a demonstrable opportunity for bugs in order to make the code hypothetically more compatible with hardware designs that are thirty years obsolete. (Hypothetical in the sense that there's little reason to believe there would be no other problems.) regards, tom lane
Re: Support for Rust
On Mon, Sep 12, 2022 at 10:29 PM Tom Lane wrote: > > Lev Kokotov writes: > > I took a small part of Postgres to get started, so just as a PoC; it > > compiles and runs though. Larger parts will take more work (deleting code, > > not just swapping object files), and more fancy things like PG_TRY() and > > friends will have to be rewritten, so not a short and easy migration. > > Yeah, that's what I thought. "Allow some parts to be written in > language X" soon turns into "Rewrite the entire system in language X, > including fundamental rethinking of memory management, error handling, > and some other things". That's pretty much a non-starter. Added "Rewrite the code in a different language" to "Features We Do Not Want" section of Wiki, referencing the two threads that came up: https://wiki.postgresql.org/wiki/Todo#Features_We_Do_Not_Want -- John Naylor EDB: http://www.enterprisedb.com
Re: Query Jumbling for CALL and SET utility statements
Hi,
On 9/16/22 5:47 PM, Drouvot, Bertrand wrote:
Hi,
On 9/16/22 2:53 PM, Fujii Masao wrote:
Attached v5 to normalize 2PC commands too, so that we get things like:
+ case T_VariableSetStmt:
+ {
+ VariableSetStmt *stmt = (VariableSetStmt *) node;
+
+ /* stmt->name is NULL for RESET ALL */
+ if (stmt->name)
+ {
+ APP_JUMB_STRING(stmt->name);
+ JumbleExpr(jstate, (Node *) stmt->args);
With the patch, "SET ... TO DEFAULT" and "RESET ..." are counted as
the same query.
Is this intentional?
Thanks for looking at the patch!
No, it is not intentional, good catch!
Which might be ok because their behavior is basically the same.
But I'm afaid which may cause users to be confused. For example, they
may fail to
find the pgss entry for RESET command they ran and just wonder why the
command was
not recorded. To avoid such confusion, how about appending stmt->kind
to the jumble?
Thought?
I think that's a good idea and will provide a new version taking care of
it (and also Sami's comments up-thread).
Please find attached v6 taking care of the remarks mentioned above.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comdiff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out
b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index ff0166fb9d..ad8cebcbad 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -272,7 +272,7 @@ FROM pg_stat_statements ORDER BY query COLLATE "C";
wal_records > $2 as wal_records_generated, +| | |
| |
wal_records >= rows as wal_records_ge_rows +| | |
| |
FROM pg_stat_statements ORDER BY query COLLATE "C"| | |
| |
- SET pg_stat_statements.track_utility = FALSE | 1 |0 | f
| f | t
+ SET pg_stat_statements.track_utility = $1 | 1 |0 | f
| f | t
UPDATE pgss_test SET b = $1 WHERE a > $2 | 1 |3 | t
| t | t
(7 rows)
@@ -423,6 +423,155 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER
BY query COLLATE "C";
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"
| 0 |0
(6 rows)
+-- PL/pgSQL procedure and pg_stat_statements.track = top
+CREATE PROCEDURE MINUS_TWO(i INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (i - 1 - 1.0)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+CREATE PROCEDURE SUM_TWO(i INTEGER, j INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (j + j)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+SET pg_stat_statements.track = 'top';
+SET pg_stat_statements.track_utility = TRUE;
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--
+
+(1 row)
+
+CALL MINUS_TWO(3);
+CALL MINUS_TWO(7);
+CALL SUM_TWO(3, 8);
+CALL SUM_TWO(7, 5);
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+query
| calls | rows
+--+---+--
+ CALL MINUS_TWO($1)
| 2 |0
+ CALL SUM_TWO($1, $2)
| 2 |0
+ SELECT pg_stat_statements_reset()
| 1 |1
+ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"
| 0 |0
+(4 rows)
+
+-- Ensure CALL is tracked even if pg_stat_statements.track_utility is FALSE
+SET pg_stat_statements.track_utility = FALSE;
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--
+
+(1 row)
+
+CALL MINUS_TWO(3);
+CALL MINUS_TWO(7);
+CALL SUM_TWO(3, 8);
+CALL SUM_TWO(7, 5);
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+query
| calls | rows
+--+---+--
+ CALL MINUS_TWO($1)
| 2 |0
+ CALL SUM_TWO($1, $2)
| 2 |0
+ SELECT pg_stat_statements_reset()
| 1 |1
+ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"
| 0 |0
+(4 rows)
+
+SET pg_stat_statements.track_utility = TRUE;
+-- PL/pgSQL pr
Re: Data is copied twice when specifying both child and parent table in publication
FYI, I'm not sure why the cfbot hasn't reported this, but the apply v9 patch failed for me on HEAD as below: [postgres@CentOS7-x64 oss_postgres_misc]$ git apply ../patches_misc/HEAD_v9-0001-Fix-data-replicated-twice-when-specifying-publish.patch --verbose Checking patch src/backend/catalog/pg_publication.c... Checking patch src/backend/commands/subscriptioncmds.c... Hunk #1 succeeded at 1917 (offset 123 lines). Checking patch src/include/catalog/pg_proc.dat... Hunk #1 succeeded at 11607 (offset -74 lines). Checking patch src/test/regress/expected/rules.out... error: while searching for: JOIN pg_attribute a ON (((a.attrelid = gpt.relid) AND (a.attnum = k.k) AS attnames, pg_get_expr(gpt.qual, gpt.relid) AS rowfilter FROM pg_publication p, LATERAL pg_get_publication_tables((p.pubname)::text) gpt(relid, attrs, qual), (pg_class c JOIN pg_namespace n ON ((n.oid = c.relnamespace))) WHERE (c.oid = gpt.relid); error: patch failed: src/test/regress/expected/rules.out:1449 error: src/test/regress/expected/rules.out: patch does not apply Checking patch src/test/subscription/t/013_partition.pl... Checking patch src/test/subscription/t/028_row_filter.pl... Checking patch src/test/subscription/t/031_column_list.pl... -- Kind Regards, Peter Smith. Fujitsu Australia.
