Re: Possible copy and past error? (\usr\backend\commands\analyze.c)
Ranier Vilela writes: > Can someone check if there is a copy and paste error, at file: > \usr\backend\commands\analyze.c, at lines 2225 and 2226? > int num_mcv = stats->attr->attstattarget; > int num_bins = stats->attr->attstattarget; No, that's intentional I believe. Those are independent variables that just happen to start out with the same value. > If they really are the same values, it could be changed to: > int num_mcv = stats->attr->attstattarget; > int num_bins = num_mcv; That would make it look like they are interdependent, which they are not. > To silence this alert. If you have a tool that complains about that coding, I think the tool needs a solid whack upside the head. There's nothing wrong with the code, and it clearly expresses the intent, which the other way doesn't. (Or in other words: it's the compiler's job to optimize away the duplicate fetch. Not the programmer's.) regards, tom lane
Re: Make MemoryContextMemAllocated() more precise
On Wed, 2020-03-18 at 15:41 -0700, Jeff Davis wrote:
> In an off-list discussion, Andres suggested that MemoryContextStats
> could be refactored to achieve this purpose, perhaps with flags to
> avoid walking through the blocks and freelists when those are not
> needed.
Attached refactoring patch. There's enough in here that warrants
discussion that I don't think this makes sense for v13 and I'm adding
it to the July commitfest.
I still think we should do something for v13, such as the originally-
proposed patch[1]. It's not critical, but it simply reports a better
number for memory consumption. Currently, the memory usage appears to
jump, often right past work mem (by a reasonable but noticable amount),
which could be confusing.
Regarding the attached patch (target v14):
* there's a new MemoryContextCount() that simply calculates the
statistics without printing anything, and returns a struct
- it supports flags to indicate which stats should be
calculated, so that some callers can avoid walking through
blocks/freelists
* it adds a new statistic for "new space" (i.e. untouched)
* it eliminates specialization of the memory context printing
- the only specialization was for generation.c to output the
number of chunks, which can be done easily enough for the
other types, too
Regards,
Jeff Davis
[1]
https://postgr.es/m/ec63d70b668818255486a83ffadc3aec492c1f57.camel%40j-davis.com
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 2a6f44a6274..9ae62dda9cd 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1776,11 +1776,24 @@ hash_agg_set_limits(double hashentrysize, uint64 input_groups, int used_bits,
static void
hash_agg_check_limits(AggState *aggstate)
{
- uint64 ngroups = aggstate->hash_ngroups_current;
- Size meta_mem = MemoryContextMemAllocated(
- aggstate->hash_metacxt, true);
- Size hash_mem = MemoryContextMemAllocated(
- aggstate->hashcontext->ecxt_per_tuple_memory, true);
+ MemoryContextCounters counters;
+ uint64 ngroups = aggstate->hash_ngroups_current;
+ uint32 flags = (MCXT_STAT_TOTALSPACE |
+ MCXT_STAT_NEWSPACE);
+ Size meta_mem;
+ Size hash_mem;
+
+ /*
+ * Consider all memory except "newspace", which is part of a block
+ * allocation by the memory context itself that aggregation has little
+ * control over. It doesn't make sense to count that against work_mem.
+ */
+ counters = MemoryContextCount(aggstate->hash_metacxt, flags, true);
+ meta_mem = counters.totalspace - counters.newspace;
+
+ counters = MemoryContextCount(
+ aggstate->hashcontext->ecxt_per_tuple_memory, flags, true);
+ hash_mem = counters.totalspace - counters.newspace;
/*
* Don't spill unless there's at least one group in the hash table so we
@@ -1838,21 +1851,25 @@ hash_agg_enter_spill_mode(AggState *aggstate)
static void
hash_agg_update_metrics(AggState *aggstate, bool from_tape, int npartitions)
{
- Size meta_mem;
- Size hash_mem;
- Size buffer_mem;
- Size total_mem;
+ MemoryContextCounters counters;
+ uint32 flags = MCXT_STAT_TOTALSPACE | MCXT_STAT_NEWSPACE;
+ Size meta_mem;
+ Size hash_mem;
+ Size buffer_mem;
+ Size total_mem;
if (aggstate->aggstrategy != AGG_MIXED &&
aggstate->aggstrategy != AGG_HASHED)
return;
/* memory for the hash table itself */
- meta_mem = MemoryContextMemAllocated(aggstate->hash_metacxt, true);
+ counters = MemoryContextCount(aggstate->hash_metacxt, flags, true);
+ meta_mem = counters.totalspace - counters.newspace;
/* memory for the group keys and transition states */
- hash_mem = MemoryContextMemAllocated(
- aggstate->hashcontext->ecxt_per_tuple_memory, true);
+ counters = MemoryContextCount(
+ aggstate->hashcontext->ecxt_per_tuple_memory, flags, true);
+ hash_mem = counters.totalspace - counters.newspace;
/* memory for read/write tape buffers, if spilled */
buffer_mem = npartitions * HASHAGG_WRITE_BUFFER_SIZE;
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index c0623f106d2..b4f3cb33ff9 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -132,6 +132,8 @@ typedef struct AllocSetContext
Size maxBlockSize; /* maximum block size */
Size nextBlockSize; /* next block size to allocate */
Size allocChunkLimit; /* effective chunk size limit */
+ Size memAllocated; /* track memory allocated for this context */
+ uint64 nChunks; /* total number of chunks */
AllocBlock keeper; /* keep this block over resets */
/* freelist this context could be put in, or -1 if not a candidate: */
int freeListIndex; /* index in context_freelists[], or -1 */
@@ -273,9 +275,8 @@ static void AllocSetReset(MemoryContext context);
static void AllocSetDelete(MemoryContext context);
static Size AllocSetGetChunkSpace(MemoryContext context, void *pointer);
static bool AllocSetIsEmpty(MemoryContext context);
-static void
Re: [PATCH] Btree BackwardScan race condition on Standby during VACUUM
On Fri, Mar 27, 2020 at 8:58 AM Michail Nikolaev wrote: > I have spent some time trying to find any possible race condition > between btree_xlog_split and _bt_walk_left… But I can’t find any. > Also, I have tried to cause any issue by putting pg_sleep put into > btree_xlog_split (between releasing and taking of locks) but without > any luck. I pushed a commit that tries to clear up some of the details around how locking works during page splits. See commit 9945ad6e. > I agree it is better to keep the same locking logic for primary and > standby in general. But it is a possible scope of another patch. It seems useful, but only up to a point. We don't need to hold locks across related atomic operations (i.e. across each phase of a page split or page deletion). In particular, the lock coupling across page levels that we perform on the primary when ascending the tree following a page split doesn't need to occur on standbys. I added something about this to the nbtree README in commit 9f83468b353. I'm not surprised that you didn't find any problems in btree_xlog_split(). It is already conservative about locking the sibling/child pages. It could hardly be more conservative (though see the code and comments at the end of btree_xlog_split(), which mention locking and backwards scans directly). -- Peter Geoghegan
proposal - psql output file write mode
Hi I am playing with pspg and inotify support. It is working pretty well and now can be nice if forwarding to output file can be configured little bit more. Now, only append mode is supported. But append mode doesn't work with pspg pager. So I propose new pset option "file_output_mode" with two possible values "append" (default, current behave) and "rewrite" (new mode). Usage: \pset file_ouput_mode rewrite In this mode, the file is opened before printing and it is closed after printing. What do you think about this proposal? Regards Pavel
Re: improve transparency of bitmap-only heap scans
On Wed, Mar 25, 2020 at 5:44 PM James Coleman wrote:
>
> On Tue, Mar 24, 2020 at 11:02 PM Amit Kapila wrote:
> >
> > On Wed, Mar 25, 2020 at 12:44 AM Tom Lane wrote:
> > >
> > > I took a quick look through this patch. While I see nothing to complain
> > > about implementation-wise, I'm a bit befuddled as to why we need this
> > > reporting when there is no comparable data provided for regular index-only
> > > scans. Over there, you just get "Heap Fetches: n", and the existing
> > > counts for bitmap scans seem to cover the same territory.
> > >
> >
> > Isn't deducing similar information ("Skipped Heap Fetches: n") there
> > is a bit easier than it is here?
>
> While I'm not the patch author so can't speak to the original thought
> process, I do think it makes sense to show it.
>
Yeah, I also see this information could be useful. It seems Tom Lane
is not entirely convinced of this. I am not sure if this is the right
time to seek more opinions as we are already near the end of CF. So,
we should either decide to move this to the next CF if we think of
getting the opinion of others or simply reject it and see a better way
for EXPLAIN to identify an index-only BMS.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: allow online change primary_conninfo
On 2020-Mar-27, Alvaro Herrera wrote: > On 2020-Mar-27, Alvaro Herrera wrote: > > > I pushed the wal_receiver_create_temp_slot bugfix, because I realized > > after looking for long enough at WalReceiverMain() that the code was > > beyond saving. I'll be pushing the rest of this later today. > > So here's the next one. I still have to go over the doc changes here, > but at least the tests are passing for me. Pushed with some documentation tweaks. Many thanks for working on this! -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Mar 26, 2020 at 11:01:06PM -0500, Justin Pryzby wrote:
> > Another issue is this:
> > > +VACUUM ( FULL [, ...] ) [ TABLESPACE > > class="parameter">new_tablespace ] [ > > class="parameter">table_and_columns [, ...] ]
> > As you mentioned in your v1 patch, in the other cases, "tablespace
> > [tablespace]" is added at the end of the command rather than in the middle.
> > I
> > wasn't able to make that work, maybe because "tablespace" isn't a fully
> > reserved word (?). I didn't try with "SET TABLESPACE", although I
> > understand
> > it'd be better without "SET".
>
> I think we should use the parenthesized syntax for vacuum - it seems clear in
> hindsight.
>
> Possibly REINDEX should use that, too, instead of adding OptTablespace at the
> end. I'm not sure.
The attached mostly implements generic parenthesized options to REINDEX(...),
so I'm soliciting opinions: should TABLESPACE be implemented in parenthesized
syntax or non?
> CLUSTER doesn't support parenthesized syntax, but .. maybe it should?
And this ?
--
Justin
>From ae8ded27088280ff399a4d1c60c7c69f40cf8723 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Mon, 23 Mar 2020 21:10:29 +0300
Subject: [PATCH v14 1/5] Allow REINDEX to change tablespace
REINDEX already does full relation rewrite, this patch adds a
possibility to specify a new tablespace where new relfilenode
will be created.
---
doc/src/sgml/ref/reindex.sgml | 24 +++-
src/backend/catalog/index.c | 89 --
src/backend/commands/cluster.c| 2 +-
src/backend/commands/indexcmds.c | 139 --
src/backend/commands/tablecmds.c | 2 +-
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c| 1 +
src/backend/parser/gram.y | 14 ++-
src/backend/tcop/utility.c| 6 +-
src/bin/psql/tab-complete.c | 6 +
src/include/catalog/index.h | 7 +-
src/include/commands/defrem.h | 6 +-
src/include/nodes/parsenodes.h| 1 +
src/test/regress/input/tablespace.source | 39 ++
src/test/regress/output/tablespace.source | 50
15 files changed, 354 insertions(+), 33 deletions(-)
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index c54a7c420d..b97bd673a5 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
-REINDEX [ ( option [, ...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURRENTLY ] name
+REINDEX [ ( option [, ...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURRENTLY ] name [ TABLESPACE new_tablespace ]
where option can be one of:
@@ -174,6 +174,28 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+TABLESPACE
+
+
+ This specifies a tablespace, where all rebuilt indexes will be created.
+ Cannot be used with "mapped" relations. If SCHEMA,
+ DATABASE or SYSTEM is specified, then
+ all unsuitable relations will be skipped and a single WARNING
+ will be generated.
+
+
+
+
+
+new_tablespace
+
+
+ The name of a specific tablespace to store rebuilt indexes.
+
+
+
+
VERBOSE
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 2d81bc3cbc..b76290e183 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1235,9 +1235,13 @@ index_create(Relation heapRelation,
* Create concurrently an index based on the definition of the one provided by
* caller. The index is inserted into catalogs and needs to be built later
* on. This is called during concurrent reindex processing.
+ *
+ * "tablespaceOid" is the new tablespace to use for this index. If
+ * InvalidOid, use the tablespace in-use instead.
*/
Oid
-index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, const char *newName)
+index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
+ Oid tablespaceOid, const char *newName)
{
Relation indexRelation;
IndexInfo *oldInfo,
@@ -1367,7 +1371,8 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, const char
newInfo,
indexColNames,
indexRelation->rd_rel->relam,
- indexRelation->rd_rel->reltablespace,
+ OidIsValid(tablespaceOid) ?
+tablespaceOid : indexRelation->rd_rel->reltablespace,
indexRelation->rd_indcollation,
indclass->values,
indcoloptions->values,
@@ -3415,10 +3420,12 @@ IndexGetRelation(Oid indexId, bool missing_ok)
/*
* reindex_index - This routine is used to recreate a single index
+ *
+ * See comments of reindex_relation() for details about "tablespaceOid".
*/
void
-reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
- int options)
+reindex_index(Oid indexId, Oid tablespaceOid, bool
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
On Fri, Mar 27, 2020 at 12:51:34PM -0400, James Coleman wrote: In a previous email I'd summarized remaining TODOs I'd found. Here's an updated listed with several resolved. Resolved: 2. Not marked in the patch, but in nodeIncrementalSort.c ExecIncrementalSort() I wonder if perhaps we should move the algorithm discussion comments up to the file header comment. On the other hand, I suppose it could be valuable to leave the the file header comment more high level about the mathematical properties of incremental sort rather than discussing the details of the hybrid mode. I've decided to do this, and the attached patch series includes the change. It's a bit tough to find the right balance what to put into the header comment and what should go to function comments, but this seems mostly reasonable. I wouldn't use the double-tab indentation and the copyright notices should stay at the top. 3. nodeIncrementalSort.c ExecIncrementalSort() in the main for loop: * TODO: do we need to check for interrupts inside these loops or * will the outer node handle that? It seems like what we have is sufficient, given that the nodes (and sort) we rely on have their own calls. The one place where someone might make an argument otherwise would be in the mode transition function where we copy tuples from the full sort state to the presorted sort state. If this is a problem, let me know, and I'll change it, but I'm proceeding under the assumption for now that it's not. I think what we have now is sufficient. 4. nodeIncrementalSort.c ExecReScanIncrementalSort: This whole chunk is suspect. I've mentioned previously I don't have a great mental model of how rescan works and its invariants (IIRC someone said it was about moving around a result set in a cursor). Regardless I'm pretty sure this code just doesn't work correctly. Additionally the sort_Done variable is poorly named; it probably would make more sense to call it something like "scanHasBegun". I'm waiting to change it though until cleaning up this code more holistically. Fixed, as described in previous email. 6. regress/expected/incremental_sort.out: -- TODO if an analyze happens here the plans might change; should we -- solve by inserting extra rows or by adding a GUC that would somehow -- forcing the time of plan we expect. I've decided this doesn't seem to be a real issue, so, comment removed. OK 7. Not listed as a comment in the patch, but I need to modify the testing for analyze output to parse out the memory/disk stats to the tests are stable. Included in the attached patch series. I use plpgsql to munge out the space kB numbers. I also discovered two bugs in the JSON output along the way and fixed those (memory and disk need to be output separate; disk was using the wrong "space type" enum). Finally I also use plpgsql to check a few invariants (for now just that max space is greater than or equal to the average. OK 8. optimizer/path/allpaths.c get_useful_pathkeys_for_relation: * XXX At the moment this can only ever return a list with a single element, * because it looks at query_pathkeys only. So we might return the pathkeys * directly, but it seems plausible we'll want to consider other orderings * in the future. I think we just leave this in as a comment. Fine with me. As a side note here, I'm wondering if this (determining useful pathkeys) can be made a bit smarter by looking both at query_pathkeys and pathkeys useful for merging, similarly to what truncate_useless_pathkeys() does. But that can be seen as an improvement of what we do now. 9. optimizer/path/allpaths.c get_useful_pathkeys_for_relation: * Considering query_pathkeys is always worth it, because it might let us * avoid a local sort. That originally was a copy from the fdw code, but since the two functions have diverged (Is that concerning? I could be confusing, but isn't a compilation problem) I didn't move the function. I think it's OK the two functions diverged, it's simply because the FDW one needs to check other things too. But I might rework this once I look closer at truncate_useless_pathkeys. I did notice though that find_em_expr_for_rel() is wholesale copied (and unchanged) from the fdw code, so I moved it to equivclass.c so both places can share it. +1 Still remaining: 1. src/backend/optimizer/util/pathnode.c add_partial_path() * XXX Perhaps we could do this only when incremental sort is enabled, * and use the simpler version (comparing just total cost) otherwise? I don't have a strong opinion here. It doesn't seem like a significant difference in terms of cost? 5. planner.c create_ordered_paths: * XXX This only looks at sort_pathkeys. I wonder if it needs to look at the * other pathkeys (grouping, ...) like generate_useful_gather_paths. 10. optimizer/path/allpaths.c generate_useful_gather_paths: * XXX I wonder if we need to consider adding a projection here, as * create_ordered_paths does. 11. In the same function as the above: * XXX Can't we
Re: pgbench - refactor init functions with buffers
On 2020-Mar-27, Tom Lane wrote: > That being the case, I'd think a better design principle is "make your > new code look like the code around it", which would tend to weigh against > introducing StringInfo uses into pgbench when there's none there now and > a bunch of PQExpBuffer instead. So I can't help thinking the advice > you're being given here is suspect. +1 for keeping it PQExpBuffer-only, until such a time when you need a StringInfo feature that's not in PQExpBuffer -- and even at that point, I think you'd switch just that one thing to StringInfo, not the whole program. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgbench - refactor init functions with buffers
Fabien COELHO writes: >>> Ok. I find it strange to mix PQExpBuffer & StringInfo in the same file. >> Agreed, but we'd rather use StringInfo going forward. However, I don't >> think >> that puts you on the hook for updating all the PQExpBuffer references. >> Unless you want to... > I cannot say that I "want" to fix something which already works the same > way, because it is against my coding principles. > However there may be some fun in writing a little script to replace one > with the other automatically. I counted nearly 3500 calls under src/bin. Yeah, that's the problem. If someone does come forward with a patch to do that, I think it'd be summarily rejected, at least in high-traffic code like pg_dump. The pain it'd cause for back-patching would outweigh the value. That being the case, I'd think a better design principle is "make your new code look like the code around it", which would tend to weigh against introducing StringInfo uses into pgbench when there's none there now and a bunch of PQExpBuffer instead. So I can't help thinking the advice you're being given here is suspect. regards, tom lane
Re: error context for vacuum to include block number
On Sat, Mar 28, 2020 at 12:34 AM Justin Pryzby wrote:
>
> On Fri, Mar 27, 2020 at 11:50:30AM +0530, Amit Kapila wrote:
> > > > The crash scenario I'm trying to avoid would be like statement_timeout
> > > > or other
> > > > asynchronous event occurring between two non-atomic operations.
> > > >
> > > +if (errinfo->phase==VACUUM_ERRCB_PHASE_VACUUM_INDEX &&
> > > errinfo->indname==NULL)
> > > +{
> > > +kill(getpid(), SIGINT);
> > > +pg_sleep(1); // that's needed since signals are delivered asynchronously
> > > +}
> > > I'm not sure if those are possible outside of "induced" errors. Maybe the
> > > function is essentially atomic due to no CHECK_FOR_INTERRUPTS or similar?
> >
> > Yes, this is exactly the point. I think unless you have
> > CHECK_FOR_INTERRUPTS in that function, the problems you are trying to
> > think won't happen.
>
> Hm, but I caused a crash *without* adding CHECK_FOR_INTERRUPTS, just
> kill+sleep. The kill() could come from running pg_cancel_backend(). And the
> sleep() just encourages a context switch, which can happen at any time.
>
pg_sleep internally uses CHECK_FOR_INTERRUPTS() due to which it would
have accepted the signal sent via pg_cancel_backend(). Can you try
your scenario by temporarily removing CHECK_FOR_INTERRUPTS from
pg_sleep() or maybe better by using OS Sleep call?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: Online checksums verification in the backend
On Wed, 18 Mar 2020 at 19:11, Julien Rouhaud wrote:
>
> On Wed, Mar 18, 2020 at 07:06:19AM +0100, Julien Rouhaud wrote:
> > On Wed, Mar 18, 2020 at 01:20:47PM +0900, Michael Paquier wrote:
> > > On Mon, Mar 16, 2020 at 09:21:22AM +0100, Julien Rouhaud wrote:
> > > > On Mon, Mar 16, 2020 at 12:29:28PM +0900, Michael Paquier wrote:
> > > >> With a large amount of
> > > >> shared buffer eviction you actually increase the risk of torn page
> > > >> reads. Instead of a logic relying on partition mapping locks, which
> > > >> could be unwise on performance grounds, did you consider different
> > > >> approaches? For example a kind of pre-emptive lock on the page in
> > > >> storage to prevent any shared buffer operation to happen while the
> > > >> block is read from storage, that would act like a barrier.
> > > >
> > > > Even with a workload having a large shared_buffers eviction pattern, I
> > > > don't
> > > > think that there's a high probability of hitting a torn page. Unless
> > > > I'm
> > > > mistaken it can only happen if all those steps happen concurrently to
> > > > doing the
> > > > block read just after releasing the LWLock:
> > > >
> > > > - postgres read the same block in shared_buffers (including all the
> > > > locking)
> > > > - dirties it
> > > > - writes part of the page
> > > >
> > > > It's certainly possible, but it seems so unlikely that the optimistic
> > > > lock-less
> > > > approach seems like a very good tradeoff.
> > >
> > > Having false reports in this area could be very confusing for the
> > > user. That's for example possible now with checksum verification and
> > > base backups.
> >
> >
> > I agree, however this shouldn't be the case here, as the block will be
> > rechecked while holding proper lock the 2nd time in case of possible false
> > positive before being reported as corrupted. So the only downside is to
> > check
> > twice a corrupted block that's not found in shared buffers (or concurrently
> > loaded/modified/half flushed). As the number of corrupted or concurrently
> > loaded/modified/half flushed blocks should usually be close to zero, it
> > seems
> > worthwhile to have a lockless check first for performance reason.
>
>
> I just noticed some dumb mistakes while adding the new GUCs. v5 attached to
> fix that, no other changes.
Thank you for updating the patch. I have some comments:
1.
+
+pg_check_relation(relation
oid, fork
text)
+
Looking at the declaration of pg_check_relation, 'relation' and 'fork'
are optional arguments. So I think the above is not correct. But as I
commented below, 'relation' should not be optional, so maybe the above
line could be:
+pg_check_relation(relation
oid[, fork
text])
2.
+
+pg_check_relation
+
+
+pg_check_relation iterates over all the blocks of all
+or the specified fork of a given relation and verify their checksum. It
+returns the list of blocks for which the found checksum doesn't match the
+expected one. You must be a member of the
+pg_read_all_stats role to use this function. It can
+only be used if data checksums are enabled. See for more information.
+
* I think we need a description about possible values for 'fork'
(i.g., 'main', 'vm', 'fsm' and 'init'), and the behavior when 'fork'
is omitted.
* Do we need to explain about checksum cost-based delay here?
3.
+CREATE OR REPLACE FUNCTION pg_check_relation(
+ IN relation regclass DEFAULT NULL::regclass, IN fork text DEFAULT NULL::text,
+ OUT relid oid, OUT forknum integer, OUT failed_blocknum bigint,
+ OUT expected_checksum integer, OUT found_checksum integer)
+ RETURNS SETOF record VOLATILE LANGUAGE internal AS 'pg_check_relation'
+ PARALLEL RESTRICTED;
Now that pg_check_relation doesn't accept NULL as 'relation', I think
we need to make 'relation' a mandatory argument.
4.
+ /* Check if the relation (still) exists */
+ if (SearchSysCacheExists1(RELOID, ObjectIdGetDatum(relid)))
+ {
+ /*
+* We use a standard relation_open() to acquire the initial lock. It
+* means that this will block until the lock is acquired, or will
+* raise an ERROR if lock_timeout has been set. If caller wants to
+* check multiple tables while relying on a maximum wait time, it
+* should process tables one by one instead of relying on a global
+* processing with the main SRF.
+*/
+ relation = relation_open(relid, AccessShareLock);
+ }
IIUC the above was necessary because we used to have
check_all_relations() which iterates all relations on the database to
do checksum checks. But now that we don't have that function and
pg_check_relation processes one relation. Can we just do
relation_open() here?
5.
I think we need to check if the relation is a temp relation. I'm not
sure it's worth to check checksums of temp relations but at least we
need not to check other's temp relations.
6.
+/*
+ * Safely read the wanted buffer from
Re: [PATCH] Check operator when creating unique index on partition table
> On Mar 26, 2020, at 01:00, Tom Lane wrote: > > Guancheng Luo writes: >> I found that things could go wrong in some cases, when the unique index and >> the partition key use different opclass. > > I agree that this is an oversight, but it seems like your solution is > overcomplicated and probably still too forgiving. Should we not just > insist that the index opfamily match the partition key opfamily? > It looks to me like that would reduce the code change to about like > this: > > - if (key->partattrs[i] == indexInfo->ii_IndexAttrNumbers[j]) > + if (key->partattrs[i] == indexInfo->ii_IndexAttrNumbers[j] && > + key->partopfamily[i] == > get_opclass_family(classObjectId[j])) > > which is a lot more straightforward and provides a lot more certainty > that the index will act as the partition constraint demands. > > This would reject, for example, a hash index associated with a btree-based > partition constraint, but I'm not sure we're losing anything much thereby. > (I do not think your patch is correct for the case where the opfamilies > belong to different AMs, anyway.) Since unique index cannot be using HASH, I think we only need to consider BTREE index here. There is cases when a BTREE index associated with a HASH partition key, but I think we should allow them, as long as their equality operators consider the same value as equal. I’ve added some more test for this case. > I'm not really on board with adding a whole new test script for this, > either. Indeed, I think `indexing.sql` might be more apporiate. I moved these tests in my new patch. 0001-Check-operator-when-creating-UNIQUE-index-on-PARTITI.patch Description: Binary data Best Regards, Guancheng Luo
Re: Berserk Autovacuum (let's save next Mandrill)
On Sat, 2020-03-28 at 11:59 +1300, David Rowley wrote: > On Fri, 27 Mar 2020 at 22:40, Laurenz Albe wrote: > > The new meaning of -2 should be documented, other than that it looks > > good to me. > > But the users don't need to know anything about -2. It's not possible > to explicitly set the value to -2. This is just the reset value of the > reloption which means "use the GUC". I see. > > I'll accept the new semantics, but they don't make me happy. People are > > used to -1 meaning "use the GUC value instead". > > The problem with having -1 on the reloption meaning use the GUC, in > this case, is that it means the reset value of the reloption must be > -1 and we need to allow them to set -2 explicitly, and if we do that, > then -1 also becomes a valid value that users can set. Maybe that's > not the end of the world, but I'd rather have the reset value be > unsettable by users. To me, that's less confusing as there are fewer > special values to remember the meaning of. > > The reason I want a method to explicitly disable the feature is the > fact that it's easy to document and it should reduce the number of > people who are confused about the best method to disable the feature. > I know there's going to be a non-zero number of people who'll want to > do that. In the light of that, I have no objections. Yours, Laurenz Albe
Re: error context for vacuum to include block number
On Sat, Mar 28, 2020 at 6:46 AM Justin Pryzby wrote: > > On Sat, Mar 28, 2020 at 06:28:38AM +0530, Amit Kapila wrote: > > > Hm, but I caused a crash *without* adding CHECK_FOR_INTERRUPTS, just > > > kill+sleep. The kill() could come from running pg_cancel_backend(). And > > > the > > > sleep() just encourages a context switch, which can happen at any time. > > > > pg_sleep internally uses CHECK_FOR_INTERRUPTS() due to which it would > > have accepted the signal sent via pg_cancel_backend(). Can you try > > your scenario by temporarily removing CHECK_FOR_INTERRUPTS from > > pg_sleep() or maybe better by using OS Sleep call? > > Ah, that explains it. Right, I'm not able to induce a crash with usleep(). > > Do you want me to resend a patch without that change ? I feel like continuing > to trade patches is more likely to introduce new errors or lose someone else's > changes than to make much progress. The patch has been through enough > iterations and it's very easy to miss an issue if I try to eyeball it. > I can do it but we have to agree on the other two points (a) I still feel that switching to the truncate phase should be done at the place from where we are calling lazy_truncate_heap and (b) lazy_cleanup_index should switch back the error phase after calling index_vacuum_cleanup. I have explained my reasoning for these points a few emails back. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: error context for vacuum to include block number
On Sat, Mar 28, 2020 at 7:04 AM Justin Pryzby wrote: > > On Sat, Mar 28, 2020 at 06:59:10AM +0530, Amit Kapila wrote: > > On Sat, Mar 28, 2020 at 6:46 AM Justin Pryzby wrote: > > > > > > On Sat, Mar 28, 2020 at 06:28:38AM +0530, Amit Kapila wrote: > > > > > Hm, but I caused a crash *without* adding CHECK_FOR_INTERRUPTS, just > > > > > kill+sleep. The kill() could come from running pg_cancel_backend(). > > > > > And the > > > > > sleep() just encourages a context switch, which can happen at any > > > > > time. > > > > > > > > pg_sleep internally uses CHECK_FOR_INTERRUPTS() due to which it would > > > > have accepted the signal sent via pg_cancel_backend(). Can you try > > > > your scenario by temporarily removing CHECK_FOR_INTERRUPTS from > > > > pg_sleep() or maybe better by using OS Sleep call? > > > > > > Ah, that explains it. Right, I'm not able to induce a crash with > > > usleep(). > > > > > > Do you want me to resend a patch without that change ? I feel like > > > continuing > > > to trade patches is more likely to introduce new errors or lose someone > > > else's > > > changes than to make much progress. The patch has been through enough > > > iterations and it's very easy to miss an issue if I try to eyeball it. > > > > I can do it but we have to agree on the other two points (a) I still > > feel that switching to the truncate phase should be done at the place > > from where we are calling lazy_truncate_heap and (b) > > lazy_cleanup_index should switch back the error phase after calling > > index_vacuum_cleanup. I have explained my reasoning for these points > > a few emails back. > > I have no objection to either. It was intuitive to me to do it how I > originally wrote it but I'm not wedded to it. > Please find attached the updated patch with all the changes discussed. Let me know if I have missed anything? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com v38-0001-Introduce-vacuum-errcontext-to-display-additiona.patch Description: Binary data
Re: WIP/PoC for parallel backup
On Wed, Mar 25, 2020 at 12:22 PM Rajkumar Raghuwanshi <
rajkumar.raghuwan...@enterprisedb.com> wrote:
> Hi Asif,
>
> While testing further I observed parallel backup is not able to take
> backup of standby server.
>
> mkdir /tmp/archive_dir
> echo "archive_mode='on'">> data/postgresql.conf
> echo "archive_command='cp %p /tmp/archive_dir/%f'">> data/postgresql.conf
>
> ./pg_ctl -D data -l logs start
> ./pg_basebackup -p 5432 -Fp -R -D /tmp/slave
>
> echo "primary_conninfo='host=127.0.0.1 port=5432 user=edb'">>
> /tmp/slave/postgresql.conf
> echo "restore_command='cp /tmp/archive_dir/%f %p'">>
> /tmp/slave/postgresql.conf
> echo "promote_trigger_file='/tmp/failover.log'">>
> /tmp/slave/postgresql.conf
>
> ./pg_ctl -D /tmp/slave -l /tmp/slave_logs -o "-p 5433" start -c
>
> [edb@localhost bin]$ ./psql postgres -p 5432 -c "select
> pg_is_in_recovery();"
> pg_is_in_recovery
> ---
> f
> (1 row)
>
> [edb@localhost bin]$ ./psql postgres -p 5433 -c "select
> pg_is_in_recovery();"
> pg_is_in_recovery
> ---
> t
> (1 row)
>
>
>
>
> *[edb@localhost bin]$ ./pg_basebackup -p 5433 -D /tmp/bkp_s --jobs
> 6pg_basebackup: error: could not list backup files: ERROR: the standby was
> promoted during online backupHINT: This means that the backup being taken
> is corrupt and should not be used. Try taking another online
> backup.pg_basebackup: removing data directory "/tmp/bkp_s"*
>
> #same is working fine without parallel backup
> [edb@localhost bin]$ ./pg_basebackup -p 5433 -D /tmp/bkp_s --jobs 1
> [edb@localhost bin]$ ls /tmp/bkp_s/PG_VERSION
> /tmp/bkp_s/PG_VERSION
>
> Thanks & Regards,
> Rajkumar Raghuwanshi
>
>
> On Thu, Mar 19, 2020 at 4:11 PM Rajkumar Raghuwanshi <
> rajkumar.raghuwan...@enterprisedb.com> wrote:
>
>> Hi Asif,
>>
>> In another scenarios, bkp data is corrupted for tablespace. again this is
>> not reproducible everytime,
>> but If I am running the same set of commands I am getting the same error.
>>
>> [edb@localhost bin]$ ./pg_ctl -D data -l logfile start
>> waiting for server to start done
>> server started
>> [edb@localhost bin]$
>> [edb@localhost bin]$ mkdir /tmp/tblsp
>> [edb@localhost bin]$ ./psql postgres -p 5432 -c "create tablespace tblsp
>> location '/tmp/tblsp';"
>> CREATE TABLESPACE
>> [edb@localhost bin]$ ./psql postgres -p 5432 -c "create database testdb
>> tablespace tblsp;"
>> CREATE DATABASE
>> [edb@localhost bin]$ ./psql testdb -p 5432 -c "create table testtbl (a
>> text);"
>> CREATE TABLE
>> [edb@localhost bin]$ ./psql testdb -p 5432 -c "insert into testtbl
>> values ('parallel_backup with tablespace');"
>> INSERT 0 1
>> [edb@localhost bin]$ ./pg_basebackup -p 5432 -D /tmp/bkp -T
>> /tmp/tblsp=/tmp/tblsp_bkp --jobs 2
>> [edb@localhost bin]$ ./pg_ctl -D /tmp/bkp -l /tmp/bkp_logs -o "-p "
>> start
>> waiting for server to start done
>> server started
>> [edb@localhost bin]$ ./psql postgres -p -c "select * from
>> pg_tablespace where spcname like 'tblsp%' or spcname = 'pg_default'";
>> oid | spcname | spcowner | spcacl | spcoptions
>> ---++--++
>> 1663 | pg_default | 10 ||
>> 16384 | tblsp | 10 ||
>> (2 rows)
>>
>> [edb@localhost bin]$ ./psql testdb -p -c "select * from testtbl";
>> psql: error: could not connect to server: FATAL:
>> "pg_tblspc/16384/PG_13_202003051/16385" is not a valid data directory
>> DETAIL: File "pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION" is
>> missing.
>> [edb@localhost bin]$
>> [edb@localhost bin]$ ls
>> data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
>> data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
>> [edb@localhost bin]$ ls
>> /tmp/bkp/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
>> ls: cannot access
>> /tmp/bkp/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION: No such file or
>> directory
>>
>>
>> Thanks & Regards,
>> Rajkumar Raghuwanshi
>>
>>
>> On Mon, Mar 16, 2020 at 6:19 PM Rajkumar Raghuwanshi <
>> rajkumar.raghuwan...@enterprisedb.com> wrote:
>>
>>> Hi Asif,
>>>
>>> On testing further, I found when taking backup with -R, pg_basebackup
>>> crashed
>>> this crash is not consistently reproducible.
>>>
>>> [edb@localhost bin]$ ./psql postgres -p 5432 -c "create table test (a
>>> text);"
>>> CREATE TABLE
>>> [edb@localhost bin]$ ./psql postgres -p 5432 -c "insert into test
>>> values ('parallel_backup with -R recovery-conf');"
>>> INSERT 0 1
>>> [edb@localhost bin]$ ./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp
>>> -R
>>> Segmentation fault (core dumped)
>>>
>>> stack trace looks the same as it was on earlier reported crash with
>>> tablespace.
>>> --stack trace
>>> [edb@localhost bin]$ gdb -q -c core.37915 pg_basebackup
>>> Loaded symbols for /lib64/libnss_files.so.2
>>> Core was generated by `./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp
>>> -R'.
>>> Program terminated with signal 11, Segmentation fault.
>>> #0 0x004099ee in worker_get_files (wstate=0xc1e458) at
>>>
Re: Unix-domain socket support on Windows
On 2/12/20 3:32 AM, Peter Eisentraut wrote: > Here is another patch set to enable this functionality. > > 0001 enables Unix-domain sockets on Windows, but leaves them turned > off by default at run time, using the mechanism introduced by > a9cff89f7e. This is relatively straightforward, except perhaps some > aesthetic questions about how these different configuration bits are > distributed around the various files. > > 0002 deals with pg_upgrade. It preserves the existing behavior of not > using Unix-domain sockets on Windows. This could perhaps be enhanced > later by either adding a command-line option or a run-time test. It's > too complicated right now. > > 0003 deals with how initdb should initialize postgresql.conf and > pg_hba.conf. It introduces a run-time test similar to how we detect > presence of IPv6. After I wrote this patch, I have come to think that > this is overkill and we should just always leave the "local" line in > pg_hba.conf even if there is no run-time support in the OS. (I think > the reason we do the run-time test for IPv6 is that we need it to > parse the IPv6 addresses in pg_hba.conf, but there is no analogous > requirement for Unix-domain sockets.) This patch is optional in any > case. > > 0004 fixes a bug in the pg_upgrade test.sh script that was exposed by > these changes. > > 0005 fixes up some issues in the test suites. Right now, the TAP > tests are hardcoded to not use Unix-domain sockets on Windows, where > as pg_regress keys off HAVE_UNIX_SOCKETS, which is no longer a useful > distinguisher. The change is to not use Unix-domain sockets for all > the tests by default on Windows (the previous behavior) but give an > option to use them. At the moment, I would consider running the test > suites with Unix-domain sockets enabled as experimental, but that's > only because of various issues in the test setups. For instance, > there is an issue in the comment of pg_regress.c remove_temp() that > I'm not sure how to address. Also, the TAP tests don't seem to work > because of some path issues. I figured I'd call time on fiddling with > this for now and ship the patches. > I have tested this on drongo/fairywren and it works fine. The patches apply cleanly (with a bit of fuzz) and a full buildfarm run is happy in both cases. Unfortunately I don't have a Windows machine that's young enough to support git master and old enough not to support Unix Domain sockets, so i can't test that with socket-enabled binaries. On inspection the patches seem fine. Let's commit this and keep working on the pg_upgrade and test issues. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: A bug when use get_bit() function for a long bytea string
Ashutosh Bapat wrote: > I think we need a similar change in byteaGetBit() and byteaSetBit() > as well. get_bit() and set_bit() as SQL functions take an int4 as the "offset" argument representing the bit number, meaning that the maximum value that can be passed is 2^31-1. But the maximum theorical size of a bytea value being 1 gigabyte or 2^30 bytes, the real maximum bit number in a bytea equals 2^33-1 (2^33=8*2^30), which doesn't fit into an "int4". As a result, the part of a bytea beyond the first 256MB is inaccessible to get_bit() and set_bit(). So aside from the integer overflow bug, isn't there the issue that the "offset" argument of get_bit() and set_bit() should have been an int8 in the first place? Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
Re: plan cache overhead on plpgsql expression
I wrote: > Amit Langote writes: >> One thing -- I don't get the division between >> CachedPlanAllowsSimpleValidityCheck() and CachedPlanIsSimplyValid(). >> Maybe I am missing something, but could there not be just one >> function, possibly using whether expr_simple_expr is set or not to >> skip or do, resp., the checks that the former does? > Well, we don't want to do the initial checks over again every time; > we want the is-valid test to be as simple and fast as we can make it. > I suppose we could have one function with a boolean flag saying "this is a > recheck", but I don't find that idea to be any better than the way it is. So after looking at the buildfarm results, I think you were on to something. The initial and recheck conditions actually have to be a bit different, and the reason is that immediately after GetCachedPlan has produced a plan, it's possible for plansource->is_valid to be false even though the derived plan is marked valid. (In the buildfarm, this is happening because of CLOBBER_CACHE_ALWAYS or equivalent cache flushes; in the real world it'd probably require sinval queue overflow to happen while building the plan.) What we want in this situation is to go ahead and use the derived plan, and then rebuild next time; that's what the pre-existing code did and it's really the only reasonable answer. It might seem better to go back and try to rebuild the plan right away, but that'd be an infinite loop in a CLOBBER_CACHE_ALWAYS build. Also, if we fail to use the derived plan at all, that amounts to disabling the "simple expression" optimization as a result of a chance sinval overflow. That's bad from a performance standpoint and it will also cause regression test output changes (since, as you previously discovered, the simple-expression path produces different CONTEXT messages for error cases --- maybe we should change that, but I don't want to be forced into it). The existing code structure can't support doing it like that, so we have to refactor to make the initial check and the recheck be separate code. Working on a patch for that now. regards, tom lane
Re: Allow auto_explain to log plans before queries are executed
Kyotaro Horiguchi-4 wrote > At Thu, 27 Feb 2020 06:27:24 +0100, Pavel Stehule > pavel.stehule@ > wrote in >> odesílatel Kyotaro Horiguchi > horikyota.ntt@ > >> napsal: > > If we need a live plan dump of a running query, We could do that using > some kind of inter-backend triggering. (I'm not sure if PG offers > inter-backend signalling facility usable by extensions..) > > =# select auto_explain.log_plan_backend(12345); > > postgresql.log: > LOG: requested plan dump: blah, blah.. > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center Did you know https://www.postgresql-archive.org/pg-show-plans-Seeing-all-execution-plans-at-once-td6129231.html ? Regards PAscal -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: psql FETCH_COUNT feature does not work with combined queries
Hi Fabien, On 1/6/20 5:20 PM, Tomas Vondra wrote: On Fri, Jul 26, 2019 at 04:13:23PM +, Fabien COELHO wrote: FETCH_COUNT does not work with combined queries, and probably has never worked since 2006. For the record, this bug has already been reported & discussed by Daniel Vérité a few months back, see: https://www.postgresql.org/message-id/flat/a0a854b6-563c-4a11-bf1c-d6c6f924004d%40manitou-mail.org Given the points of view expressed on this thread, especially Tom's view against improving the lexer used by psql to known where query ends, it looks that my documentation patch is the way to go in the short term, and that if the "always show query results" patch is committed, it might simplify a bit solving this issue as the PQexec call is turned into PQsendQuery. Assuming there's no one willing to fix the behavior (and that seems unlikely given we're in the middle of a 2020-01 commitfest) it makes sense to at least document the behavior. That being said, I think the proposed patch may be a tad too brief. I don't think we need to describe the exact behavior, of course, but maybe it'd be good to mention that the behavior depends on the type of the last command etc. Also, I don't think "combined query" is a term used anywhere in the code/docs - I think the proper term is "multi-query string". Any thoughts on Tomas' comments? I'll mark this Waiting on Author because I feel like some response and/or a new patch is needed. Regards, -- -David da...@pgmasters.net
Re: Patch: to pass query string to pg_plan_query()
Tom Lane-2 wrote > Fujii Masao > masao.fujii@.nttdata > writes: >> Does anyone object to this patch? I'm thinking to commit it separetely >> at first before committing the planning_counter_in_pg_stat_statements >> patch. > > I took a quick look through v9-0001-Pass-query-string-to-the-planner.patch > and it's fine by me. It also matches up with something I've wanted to > do for awhile, which is to make the query string available during > planning and execution so that we can produce error cursors for > run-time errors, when relevant. > > [...] > > regards, tom lane Great ! Good news ;o) Regards PAscal -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: shared-memory based stats collector
On 2020-Mar-27, Kyotaro Horiguchi wrote:
> +/*
> + * XLogArchiveWakeupEnd - Set up archiver wakeup stuff
> + */
> +void
> +XLogArchiveWakeupStart(void)
> +{
> + Latch *old_latch PG_USED_FOR_ASSERTS_ONLY;
> +
> + SpinLockAcquire(>info_lck);
> + old_latch = XLogCtl->archiverWakeupLatch;
> + XLogCtl->archiverWakeupLatch = MyLatch;
> + SpinLockRelease(>info_lck);
> + Assert (old_latch == NULL);
> +}
Comment is wrong about the function name; OTOH I don't think the
old_latch assigment in the fourth line won't work well in non-assert
builds. But why do you need those shenanigans? Surely
"Assert(XLogCtl->archiverWakeupLatch == NULL)" in the locked region
before assigning MyLatch should be sufficient and acceptable?
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgbench - refactor init functions with buffers
On 11/6/19 12:48 AM, Fabien COELHO wrote: Hello Andres, Attached v3 shorten some lines and adds "append_tablespace". A v4 which just extends the patch to newly added 'G'. I'd prefer not to expand the use of pqexpbuffer in more places, and instead rather see this use StringInfo, now that's also available to frontend programs. Franckly, one or the other does not matter much to me. FWIW, I agree with Andres with regard to using StringInfo. Also, the changes to executeStatementExpect() and adding executeStatement() do not seem to fit in with the purpose of this patch. Regards, -- -David da...@pgmasters.net
Re: pgbench - rework variable management
Hi Fabien, On 1/9/20 5:04 PM, Fabien COELHO wrote: Patch v4 is a just a rebase. Patch v5 is a rebase with some adjustements. This patch is failing on the Windows build: https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.85698 I'm not sure if this had been fixed in v3 and this is a new issue or if it has been failing all along. Either way, it should be updated. Marked Waiting on Author. BTW -- sorry if I seem to be picking on your patches but these happen to be the patches with the longest time since any activity. Regards, -- -David da...@pgmasters.net
Re: [PATCH] Btree BackwardScan race condition on Standby during VACUUM
Hello. > Probably, patch in this thread should fix this in btree_xlog_split() too? I have spent some time trying to find any possible race condition between btree_xlog_split and _bt_walk_left… But I can’t find any. Also, I have tried to cause any issue by putting pg_sleep put into btree_xlog_split (between releasing and taking of locks) but without any luck. I agree it is better to keep the same locking logic for primary and standby in general. But it is a possible scope of another patch. Thanks, Michail.
Re: INSERT ... OVERRIDING USER VALUE vs GENERATED ALWAYS identity columns
On Fri, 27 Mar 2020 at 11:29, Peter Eisentraut wrote: > > We appear to have lost track of this. Ah yes, indeed! > I have re-read everything and > expanded your patch a bit with additional documentation and comments in > the tests. I looked that over, and it all looks good to me. Regards, Dean
Re: INSERT ... OVERRIDING USER VALUE vs GENERATED ALWAYS identity columns
On 3/27/20 9:33 AM, Dean Rasheed wrote: > On Fri, 27 Mar 2020 at 11:29, Peter Eisentraut > wrote: >> >> I have re-read everything and >> expanded your patch a bit with additional documentation and comments in >> the tests. > > I looked that over, and it all looks good to me. I concur. And it matches my reading of the standard (apart from the intentional derivation). -- Vik Fearing
