public inbox for [email protected]
help / color / mirror / Atom feedReading timeline from pg_control on replication slave
7+ messages / 5 participants
[nested] [flat]
* Reading timeline from pg_control on replication slave
@ 2017-10-27 08:04 Andrey Borodin <[email protected]>
2017-10-27 22:09 ` Re: Reading timeline from pg_control on replication slave Michael Paquier <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Andrey Borodin @ 2017-10-27 08:04 UTC (permalink / raw)
To: pgsql-hackers
Hi, hackers!
I'm working on backups from replication salve in WAL-G [0]
Backups used to use result of pg_walfile_name(pg_start_backup(...)). Call to pg_start_backup() works nice, but "pg_walfile_name() cannot be executed during recovery."
This function has LSN as argument and reads TimeLineId from global state.
So I made a function[1] that, if on replica, reads timeline from pg_control file and formats WAL file name as is it was produces by pg_wal_filename(lsn).
Are there any serious dangers? Obviously, this hack is not crisp and clear. Is the risk of reading stale timeline really a problem? By reading TimeLineId from file I'm fighting those precautions in pg_walfile_name(..) which were implemented for a reason, I guess.
Thanks for reading this. I'll be happy to hear any comments on the matter.
[0] https://github.com/wal-g/wal-g/pull/32
[1] https://github.com/wal-g/wal-g/pull/32/files#diff-455316c14fb04c9f9748a0798ad151d9R158
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Reading timeline from pg_control on replication slave
2017-10-27 08:04 Reading timeline from pg_control on replication slave Andrey Borodin <[email protected]>
@ 2017-10-27 22:09 ` Michael Paquier <[email protected]>
2017-10-28 09:25 ` Re: Reading timeline from pg_control on replication slave Andrey Borodin <[email protected]>
2017-10-30 02:48 ` Re: Reading timeline from pg_control on replication slave Craig Ringer <[email protected]>
0 siblings, 2 replies; 7+ messages in thread
From: Michael Paquier @ 2017-10-27 22:09 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: pgsql-hackers
On Fri, Oct 27, 2017 at 1:04 AM, Andrey Borodin <[email protected]> wrote:
> I'm working on backups from replication salve in WAL-G [0]
> Backups used to use result of pg_walfile_name(pg_start_backup(...)). Call to pg_start_backup() works nice, but "pg_walfile_name() cannot be executed during recovery."
> This function has LSN as argument and reads TimeLineId from global state.
> So I made a function[1] that, if on replica, reads timeline from pg_control file and formats WAL file name as is it was produces by pg_wal_filename(lsn).
ThisTimeLineID is not something you can rely on for standby backends
as it is not set during recovery. That's the reason behind
pg_walfile_name disabled during recovery. There are three things
popping on top of my mind that one could think about:
1) Backups cannot be completed when started on a standby in recovery
and when stopped after the standby has been promoted, meaning that its
timeline has changed.
2) After a standby has been promoted, by using pg_start_backup, you
issue a checkpoint which makes sure that the control file gets flushed
with the new information, so when pg_start_backup returns to the
caller you should have the correct timeline number because the outer
function gets evaluated last.
3) Backups taken from cascading standbys, where a direct parent has
been promoted.
1) and 2) are actually not problems per the restrictions I am giving
above, but 3) is. If I recall correctly, when a streaming standby does
a timeline jump, a restart point is not immediately generated, so you
could have the timeline on the control file not updated to the latest
timeline value, meaning that you could have the WAL file name you use
here referring to a previous timeline and not the newest one.
In short, yes, what you are doing is definitely risky in my opinion,
particularly for complex cascading setups.
--
Michael
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Reading timeline from pg_control on replication slave
2017-10-27 08:04 Reading timeline from pg_control on replication slave Andrey Borodin <[email protected]>
2017-10-27 22:09 ` Re: Reading timeline from pg_control on replication slave Michael Paquier <[email protected]>
@ 2017-10-28 09:25 ` Andrey Borodin <[email protected]>
1 sibling, 0 replies; 7+ messages in thread
From: Andrey Borodin @ 2017-10-28 09:25 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
Hi, Michael!
Thank you very much for these comments!
> 28 окт. 2017 г., в 3:09, Michael Paquier <[email protected]> написал(а):
>
> ThisTimeLineID is not something you can rely on for standby backends
> as it is not set during recovery. That's the reason behind
> pg_walfile_name disabled during recovery. There are three things
> popping on top of my mind that one could think about:
> 1) Backups cannot be completed when started on a standby in recovery
> and when stopped after the standby has been promoted, meaning that its
> timeline has changed.
> 2) After a standby has been promoted, by using pg_start_backup, you
> issue a checkpoint which makes sure that the control file gets flushed
> with the new information, so when pg_start_backup returns to the
> caller you should have the correct timeline number because the outer
> function gets evaluated last.
> 3) Backups taken from cascading standbys, where a direct parent has
> been promoted.
>
> 1) and 2) are actually not problems per the restrictions I am giving
> above, but 3) is. If I recall correctly, when a streaming standby does
> a timeline jump, a restart point is not immediately generated, so you
> could have the timeline on the control file not updated to the latest
> timeline value, meaning that you could have the WAL file name you use
> here referring to a previous timeline and not the newest one.
>
> In short, yes, what you are doing is definitely risky in my opinion,
> particularly for complex cascading setups.
We are using TimeLineId from pg_control only to give a name to backup. Slightly stale timeline Id will not incur significant problems as long as pg_control is picked up after backup finalization.
But from your words I see that the safest option is to check timeline from pg_control after start and after stop. If this timelines differ - invalidate backup entirely. This does not seem too hard condition for invalidation, does it?
Best regards, Andrey Borodin.
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Reading timeline from pg_control on replication slave
2017-10-27 08:04 Reading timeline from pg_control on replication slave Andrey Borodin <[email protected]>
2017-10-27 22:09 ` Re: Reading timeline from pg_control on replication slave Michael Paquier <[email protected]>
@ 2017-10-30 02:48 ` Craig Ringer <[email protected]>
2017-10-30 06:29 ` Re: Reading timeline from pg_control on replication slave Michael Paquier <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Craig Ringer @ 2017-10-30 02:48 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andrey Borodin <[email protected]>; pgsql-hackers
On 28 October 2017 at 06:09, Michael Paquier <[email protected]> wrote:
> On Fri, Oct 27, 2017 at 1:04 AM, Andrey Borodin <[email protected]> wrote:
>> I'm working on backups from replication salve in WAL-G [0]
>> Backups used to use result of pg_walfile_name(pg_start_backup(...)). Call to pg_start_backup() works nice, but "pg_walfile_name() cannot be executed during recovery."
>> This function has LSN as argument and reads TimeLineId from global state.
>> So I made a function[1] that, if on replica, reads timeline from pg_control file and formats WAL file name as is it was produces by pg_wal_filename(lsn).
>
> ThisTimeLineID is not something you can rely on for standby backends
> as it is not set during recovery.
That's not much of a concern really, you just have to ensure you call
GetXLogReplayRecPtr and set ThisTimeLineID.
(I'd quite like ThisTimeLineID to go away as a global. It's messy and
confusing, and I'd much rather it be fetched when needed).
However, that doesn't negate the rest of the issues you raised.
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Reading timeline from pg_control on replication slave
2017-10-27 08:04 Reading timeline from pg_control on replication slave Andrey Borodin <[email protected]>
2017-10-27 22:09 ` Re: Reading timeline from pg_control on replication slave Michael Paquier <[email protected]>
2017-10-30 02:48 ` Re: Reading timeline from pg_control on replication slave Craig Ringer <[email protected]>
@ 2017-10-30 06:29 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Michael Paquier @ 2017-10-30 06:29 UTC (permalink / raw)
To: Craig Ringer <[email protected]>; +Cc: Andrey Borodin <[email protected]>; pgsql-hackers
On Mon, Oct 30, 2017 at 2:48 AM, Craig Ringer <[email protected]> wrote:
> (I'd quite like ThisTimeLineID to go away as a global. It's messy and
> confusing, and I'd much rather it be fetched when needed).
Yeah, I agree. My take on the matter is that we could just use the
status data of the control file which is in shared memory as the only
writers to it are the checkpointer and the startup processes.
--
Michael
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v1 2/3] preserve indisclustered on children of clustered, partitioned indexes
@ 2020-10-07 01:40 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Justin Pryzby @ 2020-10-07 01:40 UTC (permalink / raw)
Note, this takes a parentIndex, but that wasn't previously used ...
UpdateIndexRelation(Oid indexoid, Oid heapoid, Oid parentIndexId,
---
src/backend/catalog/index.c | 2 +-
src/test/regress/expected/cluster.out | 11 +++++++++++
src/test/regress/sql/cluster.sql | 3 +++
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 3ce6eb61d5..92ca0927fa 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -601,7 +601,7 @@ UpdateIndexRelation(Oid indexoid,
values[Anum_pg_index_indisprimary - 1] = BoolGetDatum(primary);
values[Anum_pg_index_indisexclusion - 1] = BoolGetDatum(isexclusion);
values[Anum_pg_index_indimmediate - 1] = BoolGetDatum(immediate);
- values[Anum_pg_index_indisclustered - 1] = BoolGetDatum(false);
+ values[Anum_pg_index_indisclustered - 1] = BoolGetDatum(OidIsValid(parentIndexId) && get_index_isclustered(parentIndexId));
values[Anum_pg_index_indisvalid - 1] = BoolGetDatum(isvalid);
values[Anum_pg_index_indcheckxmin - 1] = BoolGetDatum(false);
values[Anum_pg_index_indisready - 1] = BoolGetDatum(isready);
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index e4448350e7..8f245da46d 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -495,6 +495,17 @@ Indexes:
"clstrpart_idx" btree (a) CLUSTER
Number of partitions: 3 (Use \d+ to list them.)
+CREATE TABLE clstrpart4 PARTITION OF clstrpart FOR VALUES FROM (30)TO(40);
+\d clstrpart4
+ Table "public.clstrpart4"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart FOR VALUES FROM (30) TO (40)
+Indexes:
+ "clstrpart4_a_idx" btree (a) CLUSTER
+
+DROP TABLE clstrpart;
-- Test CLUSTER with external tuplesorting
create table clstr_4 as select * from tenk1;
create index cluster_sort on clstr_4 (hundred, thousand, tenthous);
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 22225dc924..b871ab53c3 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -220,6 +220,9 @@ CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitio
CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
\d clstrpart
+CREATE TABLE clstrpart4 PARTITION OF clstrpart FOR VALUES FROM (30)TO(40);
+\d clstrpart4
+DROP TABLE clstrpart;
-- Test CLUSTER with external tuplesorting
--
2.17.0
--8Tx+BDMK09J610+l
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v1-0003-Invalidate-indisclustered-when-attaching-uncluste.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH 2/6] Refactor index_concurrently_create_copy() for use with REPACK (CONCURRENTLY).
@ 2026-01-27 10:48 Antonin Houska <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Antonin Houska @ 2026-01-27 10:48 UTC (permalink / raw)
This patch moves the code to index_create_copy() and adds a "concurrently"
parameter so it can be used by REPACK (CONCURRENTLY).
With the CONCURRENTLY option, REPACK cannot simply swap the heap file and
rebuild its indexes. Instead, it needs to build a separate set of indexes
(including system catalog entries) *before* the actual swap, to reduce the
time AccessExclusiveLock needs to be held for.
---
src/backend/catalog/index.c | 54 +++++++++++++++++++++++---------
src/backend/commands/indexcmds.c | 6 ++--
src/backend/nodes/makefuncs.c | 9 +++---
src/include/catalog/index.h | 3 ++
src/include/nodes/makefuncs.h | 4 ++-
5 files changed, 54 insertions(+), 22 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5ee6389d39c..f8e6c3d804e 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1288,15 +1288,32 @@ index_create(Relation heapRelation,
/*
* index_concurrently_create_copy
*
- * 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 tablespace to use for this index.
+ * Variant of index_create_copy(), called during concurrent reindex
+ * processing.
*/
Oid
index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
Oid tablespaceOid, const char *newName)
+{
+ return index_create_copy(heapRelation, oldIndexId, tablespaceOid, newName,
+ true);
+}
+
+/*
+ * index_create_copy
+ *
+ * Create an index based on the definition of the one provided by caller. The
+ * index is inserted into catalogs. If 'concurrently' is TRUE, it needs to be
+ * built later on, otherwise it's built immediately.
+ *
+ * "tablespaceOid" is the tablespace to use for this index.
+ *
+ * The actual implementation of index_concurrently_create_copy(), reusable for
+ * other purposes.
+ */
+Oid
+index_create_copy(Relation heapRelation, Oid oldIndexId, Oid tablespaceOid,
+ const char *newName, bool concurrently)
{
Relation indexRelation;
IndexInfo *oldInfo,
@@ -1315,6 +1332,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
List *indexColNames = NIL;
List *indexExprs = NIL;
List *indexPreds = NIL;
+ int flags = 0;
indexRelation = index_open(oldIndexId, RowExclusiveLock);
@@ -1325,7 +1343,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
* Concurrent build of an index with exclusion constraints is not
* supported.
*/
- if (oldInfo->ii_ExclusionOps != NULL)
+ if (oldInfo->ii_ExclusionOps != NULL && concurrently)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("concurrent index creation for exclusion constraints is not supported")));
@@ -1381,9 +1399,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
}
/*
- * Build the index information for the new index. Note that rebuild of
- * indexes with exclusion constraints is not supported, hence there is no
- * need to fill all the ii_Exclusion* fields.
+ * Build the index information for the new index.
*/
newInfo = makeIndexInfo(oldInfo->ii_NumIndexAttrs,
oldInfo->ii_NumIndexKeyAttrs,
@@ -1392,10 +1408,13 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
indexPreds,
oldInfo->ii_Unique,
oldInfo->ii_NullsNotDistinct,
- false, /* not ready for inserts */
- true,
+ !concurrently, /* isready */
+ concurrently, /* concurrent */
indexRelation->rd_indam->amsummarizing,
- oldInfo->ii_WithoutOverlaps);
+ oldInfo->ii_WithoutOverlaps,
+ oldInfo->ii_ExclusionOps,
+ oldInfo->ii_ExclusionProcs,
+ oldInfo->ii_ExclusionStrats);
/*
* Extract the list of column names and the column numbers for the new
@@ -1433,6 +1452,9 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
stattargets[i].isnull = isnull;
}
+ if (concurrently)
+ flags = INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT;
+
/*
* Now create the new index.
*
@@ -1456,7 +1478,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
indcoloptions->values,
stattargets,
reloptionsDatum,
- INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT,
+ flags,
0,
true, /* allow table to be a system catalog? */
false, /* is_internal? */
@@ -2450,7 +2472,8 @@ BuildIndexInfo(Relation index)
indexStruct->indisready,
false,
index->rd_indam->amsummarizing,
- indexStruct->indisexclusion && indexStruct->indisunique);
+ indexStruct->indisexclusion && indexStruct->indisunique,
+ NULL, NULL, NULL);
/* fill in attribute numbers */
for (i = 0; i < numAtts; i++)
@@ -2510,7 +2533,8 @@ BuildDummyIndexInfo(Relation index)
indexStruct->indisready,
false,
index->rd_indam->amsummarizing,
- indexStruct->indisexclusion && indexStruct->indisunique);
+ indexStruct->indisexclusion && indexStruct->indisunique,
+ NULL, NULL, NULL);
/* fill in attribute numbers */
for (i = 0; i < numAtts; i++)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 635679cc1f2..34209bd1393 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -243,7 +243,8 @@ CheckIndexCompatible(Oid oldId,
*/
indexInfo = makeIndexInfo(numberOfAttributes, numberOfAttributes,
accessMethodId, NIL, NIL, false, false,
- false, false, amsummarizing, isWithoutOverlaps);
+ false, false, amsummarizing, isWithoutOverlaps,
+ NULL, NULL, NULL);
typeIds = palloc_array(Oid, numberOfAttributes);
collationIds = palloc_array(Oid, numberOfAttributes);
opclassIds = palloc_array(Oid, numberOfAttributes);
@@ -930,7 +931,8 @@ DefineIndex(ParseState *pstate,
!concurrent,
concurrent,
amissummarizing,
- stmt->iswithoutoverlaps);
+ stmt->iswithoutoverlaps,
+ NULL, NULL, NULL);
typeIds = palloc_array(Oid, numberOfAttributes);
collationIds = palloc_array(Oid, numberOfAttributes);
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 2caec621d73..ca7e21e8349 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -834,7 +834,8 @@ IndexInfo *
makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions,
List *predicates, bool unique, bool nulls_not_distinct,
bool isready, bool concurrent, bool summarizing,
- bool withoutoverlaps)
+ bool withoutoverlaps, Oid *exclusion_ops, Oid *exclusion_procs,
+ uint16 *exclusion_strats)
{
IndexInfo *n = makeNode(IndexInfo);
@@ -863,9 +864,9 @@ makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions,
n->ii_PredicateState = NULL;
/* exclusion constraints */
- n->ii_ExclusionOps = NULL;
- n->ii_ExclusionProcs = NULL;
- n->ii_ExclusionStrats = NULL;
+ n->ii_ExclusionOps = exclusion_ops;
+ n->ii_ExclusionProcs = exclusion_procs;
+ n->ii_ExclusionStrats = exclusion_strats;
/* speculative inserts */
n->ii_UniqueOps = NULL;
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index b259c4141ed..3426087b445 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -99,6 +99,9 @@ extern Oid index_concurrently_create_copy(Relation heapRelation,
Oid oldIndexId,
Oid tablespaceOid,
const char *newName);
+extern Oid index_create_copy(Relation heapRelation, Oid oldIndexId,
+ Oid tablespaceOid, const char *newName,
+ bool concurrently);
extern void index_concurrently_build(Oid heapRelationId,
Oid indexRelationId);
diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h
index 982ec25ae14..dcea148ae1a 100644
--- a/src/include/nodes/makefuncs.h
+++ b/src/include/nodes/makefuncs.h
@@ -99,7 +99,9 @@ extern IndexInfo *makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid,
List *expressions, List *predicates,
bool unique, bool nulls_not_distinct,
bool isready, bool concurrent,
- bool summarizing, bool withoutoverlaps);
+ bool summarizing, bool withoutoverlaps,
+ Oid *exclusion_ops, Oid *exclusion_procs,
+ uint16 *exclusion_strats);
extern Node *makeStringConst(char *str, int location);
extern DefElem *makeDefElem(char *name, Node *arg, int location);
--
2.47.3
--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
filename=v32-0003-Move-conversion-of-a-historic-to-MVCC-snapshot-to-a-.patch
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-01-27 10:48 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-10-27 08:04 Reading timeline from pg_control on replication slave Andrey Borodin <[email protected]>
2017-10-27 22:09 ` Michael Paquier <[email protected]>
2017-10-28 09:25 ` Andrey Borodin <[email protected]>
2017-10-30 02:48 ` Craig Ringer <[email protected]>
2017-10-30 06:29 ` Michael Paquier <[email protected]>
2020-10-07 01:40 [PATCH v1 2/3] preserve indisclustered on children of clustered, partitioned indexes Justin Pryzby <[email protected]>
2026-01-27 10:48 [PATCH 2/6] Refactor index_concurrently_create_copy() for use with REPACK (CONCURRENTLY). Antonin Houska <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox