public inbox for [email protected]
help / color / mirror / Atom feedIn-placre persistance change of a relation
10+ messages / 5 participants
[nested] [flat]
* In-placre persistance change of a relation
@ 2020-11-11 08:33 Kyotaro Horiguchi <[email protected]>
0 siblings, 3 replies; 10+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-11 08:33 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hello. This is a thread for an alternative solution to wal_level=none
[*1] for bulk data loading.
*1: https://www.postgresql.org/message-id/TYAPR01MB29901EBE5A3ACCE55BA99186FE320%40TYAPR01MB2990.jpnprd0...
At Tue, 10 Nov 2020 09:33:12 -0500, Stephen Frost <[email protected]> wrote in
> Greetings,
>
> * Kyotaro Horiguchi ([email protected]) wrote:
> > For fuel(?) of the discussion, I tried a very-quick PoC for in-place
> > ALTER TABLE SET LOGGED/UNLOGGED and resulted as attached. After some
> > trials of several ways, I drifted to the following way after poking
> > several ways.
> >
> > 1. Flip BM_PERMANENT of active buffers
> > 2. adding/removing init fork
> > 3. sync files,
> > 4. Flip pg_class.relpersistence.
> >
> > It always skips table copy in the SET UNLOGGED case, and only when
> > wal_level=minimal in the SET LOGGED case. Crash recovery seems
> > working by some brief testing by hand.
>
> Somehow missed that this patch more-or-less does what I was referring to
> down-thread, but I did want to mention that it looks like it's missing a
> necessary FlushRelationBuffers() call before the sync, otherwise there
> could be dirty buffers for the relation that's being set to LOGGED (with
> wal_level=minimal), which wouldn't be good. See the comments above
> smgrimmedsync().
Right. Thanks. However, since SetRelFileNodeBuffersPersistence()
called just above scans shared buffers so I don't want to just call
FlushRelationBuffers() separately. Instead, I added buffer-flush to
SetRelFileNodeBuffersPersistence().
FWIW this is a revised version of the PoC, which has some known
problems.
- Flipping of Buffer persistence is not WAL-logged nor even be able to
be safely roll-backed. (It might be better to drop buffers).
- This version handles indexes but not yet handle toast relatins.
- tableAMs are supposed to support this feature. (but I'm not sure
it's worth allowing them not to do so).
> > Of course, I haven't performed intensive test on it.
>
> Reading through the thread, it didn't seem very clear, but we should
> definitely make sure that it does the right thing on replicas when going
> between unlogged and logged (and between logged and unlogged too), of
> course.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
[text/x-patch] PoC_in-place_set_persistence_v2.patch (23.8K, ../../[email protected]/2-PoC_in-place_set_persistence_v2.patch)
download | inline diff:
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index dcaea7135f..0c6ce70484 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -613,6 +613,27 @@ heapam_relation_set_new_filenode(Relation rel,
smgrclose(srel);
}
+static void
+heapam_relation_set_persistence(Relation rel, char persistence)
+{
+ Assert(rel->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT ||
+ rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
+
+ Assert (rel->rd_rel->relpersistence != persistence);
+
+ if (persistence == RELPERSISTENCE_UNLOGGED)
+ {
+ Assert(rel->rd_rel->relkind == RELKIND_RELATION ||
+ rel->rd_rel->relkind == RELKIND_MATVIEW ||
+ rel->rd_rel->relkind == RELKIND_TOASTVALUE);
+
+ RelationCreateInitFork(rel->rd_node, false);
+ }
+ else
+ RelationDropInitFork(rel->rd_node);
+}
+
+
static void
heapam_relation_nontransactional_truncate(Relation rel)
{
@@ -2540,6 +2561,7 @@ static const TableAmRoutine heapam_methods = {
.compute_xid_horizon_for_tuples = heap_compute_xid_horizon_for_tuples,
.relation_set_new_filenode = heapam_relation_set_new_filenode,
+ .relation_set_persistence = heapam_relation_set_persistence,
.relation_nontransactional_truncate = heapam_relation_nontransactional_truncate,
.relation_copy_data = heapam_relation_copy_data,
.relation_copy_for_cluster = heapam_relation_copy_for_cluster,
diff --git a/src/backend/access/rmgrdesc/smgrdesc.c b/src/backend/access/rmgrdesc/smgrdesc.c
index a7c0cb1bc3..8397002613 100644
--- a/src/backend/access/rmgrdesc/smgrdesc.c
+++ b/src/backend/access/rmgrdesc/smgrdesc.c
@@ -40,6 +40,14 @@ smgr_desc(StringInfo buf, XLogReaderState *record)
xlrec->blkno, xlrec->flags);
pfree(path);
}
+ else if (info == XLOG_SMGR_UNLINK)
+ {
+ xl_smgr_unlink *xlrec = (xl_smgr_unlink *) rec;
+ char *path = relpathperm(xlrec->rnode, xlrec->forkNum);
+
+ appendStringInfoString(buf, path);
+ pfree(path);
+ }
}
const char *
@@ -55,6 +63,9 @@ smgr_identify(uint8 info)
case XLOG_SMGR_TRUNCATE:
id = "TRUNCATE";
break;
+ case XLOG_SMGR_UNLINK:
+ id = "UNLINK";
+ break;
}
return id;
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index d538f25726..ac5aea3d38 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -60,6 +60,8 @@ int wal_skip_threshold = 2048; /* in kilobytes */
typedef struct PendingRelDelete
{
RelFileNode relnode; /* relation that may need to be deleted */
+ bool deleteinitfork; /* delete only init fork if true */
+ bool createinitfork; /* create init fork if true */
BackendId backend; /* InvalidBackendId if not a temp rel */
bool atCommit; /* T=delete at commit; F=delete at abort */
int nestLevel; /* xact nesting level of request */
@@ -153,6 +155,8 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
pending = (PendingRelDelete *)
MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
pending->relnode = rnode;
+ pending->deleteinitfork = false;
+ pending->createinitfork = false;
pending->backend = backend;
pending->atCommit = false; /* delete if abort */
pending->nestLevel = GetCurrentTransactionNestLevel();
@@ -168,6 +172,95 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
return srel;
}
+/*
+ * RelationCreateInitFork
+ * Create physical storage for a relation.
+ *
+ * Create the underlying disk file storage for the relation. This only
+ * creates the main fork; additional forks are created lazily by the
+ * modules that need them.
+ *
+ * This function is transactional. The creation is WAL-logged, and if the
+ * transaction aborts later on, the storage will be destroyed.
+ */
+void
+RelationCreateInitFork(RelFileNode rnode, bool isRedo)
+{
+ PendingRelDelete *pending;
+ SMgrRelation srel;
+ PendingRelDelete *prev;
+ PendingRelDelete *next;
+
+ prev = NULL;
+ for (pending = pendingDeletes; pending != NULL; pending = next)
+ {
+ next = pending->next;
+ if (RelFileNodeEquals(rnode, pending->relnode) &&
+ pending->deleteinitfork && pending->atCommit)
+ {
+ /* unlink and delete list entry */
+ if (prev)
+ prev->next = next;
+ else
+ pendingDeletes = next;
+ pfree(pending);
+ return;
+ }
+ else
+ {
+ /* unrelated entry, don't touch it */
+ prev = pending;
+ }
+ }
+ srel = smgropen(rnode, InvalidBackendId);
+ smgrcreate(srel, INIT_FORKNUM, isRedo);
+ if (!isRedo)
+ log_smgrcreate(&rnode, INIT_FORKNUM);
+ smgrimmedsync(srel, INIT_FORKNUM);
+
+ /* Add the relation to the list of stuff to delete at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->deleteinitfork = true;
+ pending->createinitfork = false;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false; /* delete if abort */
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+}
+
+void
+RelationDropInitFork(RelFileNode rnode)
+{
+ PendingRelDelete *pending;
+ PendingRelDelete *next;
+
+ for (pending = pendingDeletes; pending != NULL; pending = next)
+ {
+ next = pending->next;
+ if (RelFileNodeEquals(rnode, pending->relnode) &&
+ pending->deleteinitfork && pending->atCommit)
+ {
+ /* We're done. */
+ return;
+ }
+ }
+
+ /* Add the relation to the list of stuff to delete at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->deleteinitfork = true;
+ pending->createinitfork = false;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = true; /* create if abort */
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+}
+
/*
* Perform XLogInsert of an XLOG_SMGR_CREATE record to WAL.
*/
@@ -187,6 +280,25 @@ log_smgrcreate(const RelFileNode *rnode, ForkNumber forkNum)
XLogInsert(RM_SMGR_ID, XLOG_SMGR_CREATE | XLR_SPECIAL_REL_UPDATE);
}
+/*
+ * Perform XLogInsert of an XLOG_SMGR_UNLINK record to WAL.
+ */
+void
+log_smgrunlink(const RelFileNode *rnode, ForkNumber forkNum)
+{
+ xl_smgr_unlink xlrec;
+
+ /*
+ * Make an XLOG entry reporting the file unlink.
+ */
+ xlrec.rnode = *rnode;
+ xlrec.forkNum = forkNum;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+ XLogInsert(RM_SMGR_ID, XLOG_SMGR_UNLINK | XLR_SPECIAL_REL_UPDATE);
+}
+
/*
* RelationDropStorage
* Schedule unlinking of physical storage at transaction commit.
@@ -200,6 +312,8 @@ RelationDropStorage(Relation rel)
pending = (PendingRelDelete *)
MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
pending->relnode = rel->rd_node;
+ pending->createinitfork = false;
+ pending->deleteinitfork = false;
pending->backend = rel->rd_backend;
pending->atCommit = true; /* delete if commit */
pending->nestLevel = GetCurrentTransactionNestLevel();
@@ -626,19 +740,27 @@ smgrDoPendingDeletes(bool isCommit)
srel = smgropen(pending->relnode, pending->backend);
- /* allocate the initial array, or extend it, if needed */
- if (maxrels == 0)
+ if (pending->deleteinitfork)
{
- maxrels = 8;
- srels = palloc(sizeof(SMgrRelation) * maxrels);
+ log_smgrunlink(&pending->relnode, INIT_FORKNUM);
+ smgrunlink(srel, INIT_FORKNUM, false);
}
- else if (maxrels <= nrels)
+ else
{
- maxrels *= 2;
- srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
- }
+ /* allocate the initial array, or extend it, if needed */
+ if (maxrels == 0)
+ {
+ maxrels = 8;
+ srels = palloc(sizeof(SMgrRelation) * maxrels);
+ }
+ else if (maxrels <= nrels)
+ {
+ maxrels *= 2;
+ srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
+ }
- srels[nrels++] = srel;
+ srels[nrels++] = srel;
+ }
}
/* must explicitly free the list entry */
pfree(pending);
@@ -917,6 +1039,14 @@ smgr_redo(XLogReaderState *record)
reln = smgropen(xlrec->rnode, InvalidBackendId);
smgrcreate(reln, xlrec->forkNum, true);
}
+ else if (info == XLOG_SMGR_UNLINK)
+ {
+ xl_smgr_unlink *xlrec = (xl_smgr_unlink *) XLogRecGetData(record);
+ SMgrRelation reln;
+
+ reln = smgropen(xlrec->rnode, InvalidBackendId);
+ smgrunlink(reln, xlrec->forkNum, true);
+ }
else if (info == XLOG_SMGR_TRUNCATE)
{
xl_smgr_truncate *xlrec = (xl_smgr_truncate *) XLogRecGetData(record);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e3cfaf8b07..e358174b01 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4918,6 +4918,137 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
return newcmd;
}
+static bool
+try_inplace_persistence_change(AlteredTableInfo *tab, char persistence,
+ LOCKMODE lockmode)
+{
+ Relation rel;
+ Relation classRel;
+ HeapTuple tuple,
+ newtuple;
+ Datum new_val[Natts_pg_class];
+ bool new_null[Natts_pg_class],
+ new_repl[Natts_pg_class];
+ int i;
+ List *relids;
+ ListCell *lc_oid;
+
+ Assert(tab->rewrite == AT_REWRITE_ALTER_PERSISTENCE);
+ Assert(lockmode == AccessExclusiveLock);
+
+ /*
+ * Under the following condition, we need to call ATRewriteTable, which
+ * cannot be false in the AT_REWRITE_ALTER_PERSISTENCE case.
+ */
+ Assert(tab->constraints == NULL && tab->partition_constraint == NULL &&
+ tab->newvals == NULL && !tab->verify_new_notnull);
+
+ /*
+ * When wal_level is replica or higher we need that the initial state of
+ * the relation be recoverable from WAL. When wal_level >= replica
+ * switching to PERMANENT needs to emit the WAL records to reconstruct the
+ * current data. This could be done by writing XLOG_FPI for all pages but
+ * it is not obvious that that is performant than normal rewriting.
+ * Otherwise what we need for the relation data is just establishing
+ * initial state on storage and no need of WAL to reconstruct it.
+ */
+ if (tab->newrelpersistence == RELPERSISTENCE_PERMANENT && XLogIsNeeded())
+ return false;
+
+ rel = table_open(tab->relid, lockmode);
+
+ Assert(rel->rd_rel->relpersistence != persistence);
+
+ elog(DEBUG1, "perform im-place persistnce change");
+
+ RelationOpenSmgr(rel);
+
+ /* Change persistence then flush-out buffers of the relation */
+
+ /* Get the list of index OIDs for this relation */
+ relids = RelationGetIndexList(rel);
+ relids = lcons_oid(rel->rd_id, relids);
+
+ table_close(rel, lockmode);
+
+ /* Done change on storage. Update catalog including indexes. */
+ /* add the heap oid to the relation ID list */
+
+ classRel = table_open(RelationRelationId, RowExclusiveLock);
+
+ foreach (lc_oid, relids)
+ {
+ Oid reloid = lfirst_oid(lc_oid);
+ Relation r = relation_open(reloid, lockmode);
+
+ RelationOpenSmgr(r);
+
+ if (persistence == RELPERSISTENCE_UNLOGGED)
+ {
+ RelationCreateInitFork(r->rd_node, false);
+
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+ r->rd_indam->ambuildempty(r);
+ else
+ {
+ Assert(r->rd_rel->relkind == RELKIND_RELATION ||
+ r->rd_rel->relkind == RELKIND_MATVIEW ||
+ r->rd_rel->relkind == RELKIND_TOASTVALUE);
+ }
+ }
+ else
+ RelationDropInitFork(r->rd_node);
+
+ table_close(r, NoLock);
+
+ /*
+ * This relation is now WAL-logged. Sync all files immediately to
+ * establish the initial state on storgae.
+ */
+ if (persistence == RELPERSISTENCE_PERMANENT)
+ {
+ for (i = 0 ; i < MAX_FORKNUM ; i++)
+ {
+ if (smgrexists(r->rd_smgr, i))
+ smgrimmedsync(r->rd_smgr, i);
+ }
+ }
+
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+
+ memset(new_val, 0, sizeof(new_val));
+ memset(new_null, false, sizeof(new_null));
+ memset(new_repl, false, sizeof(new_repl));
+
+ new_val[Anum_pg_class_relpersistence - 1] = CharGetDatum(persistence);
+ new_null[Anum_pg_class_relpersistence - 1] = false;
+ new_repl[Anum_pg_class_relpersistence - 1] = true;
+
+ newtuple = heap_modify_tuple(tuple, RelationGetDescr(classRel),
+ new_val, new_null, new_repl);
+
+ CatalogTupleUpdate(classRel, &newtuple->t_self, newtuple);
+ heap_freetuple(newtuple);
+ }
+
+ foreach (lc_oid, relids)
+ {
+ Oid reloid = lfirst_oid(lc_oid);
+ Relation r = relation_open(reloid, lockmode);
+
+ RelationOpenSmgr(r);
+ SetRelationBuffersPersistence(r, persistence == RELPERSISTENCE_PERMANENT);
+ table_close(r, NoLock);
+ }
+ table_close(classRel, RowExclusiveLock);
+
+ return true;
+}
+
/*
* ATRewriteTables: ALTER TABLE phase 3
*/
@@ -5038,45 +5169,51 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
tab->relid,
tab->rewrite);
- /*
- * Create transient table that will receive the modified data.
- *
- * Ensure it is marked correctly as logged or unlogged. We have
- * to do this here so that buffers for the new relfilenode will
- * have the right persistence set, and at the same time ensure
- * that the original filenode's buffers will get read in with the
- * correct setting (i.e. the original one). Otherwise a rollback
- * after the rewrite would possibly result with buffers for the
- * original filenode having the wrong persistence setting.
- *
- * NB: This relies on swap_relation_files() also swapping the
- * persistence. That wouldn't work for pg_class, but that can't be
- * unlogged anyway.
- */
- OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, persistence,
- lockmode);
+ if (tab->rewrite != AT_REWRITE_ALTER_PERSISTENCE ||
+ !try_inplace_persistence_change(tab, persistence, lockmode))
+ {
+ /*
+ * Create transient table that will receive the modified data.
+ *
+ * Ensure it is marked correctly as logged or unlogged. We
+ * have to do this here so that buffers for the new relfilenode
+ * will have the right persistence set, and at the same time
+ * ensure that the original filenode's buffers will get read in
+ * with the correct setting (i.e. the original one). Otherwise
+ * a rollback after the rewrite would possibly result with
+ * buffers for the original filenode having the wrong
+ * persistence setting.
+ *
+ * NB: This relies on swap_relation_files() also swapping the
+ * persistence. That wouldn't work for pg_class, but that can't
+ * be unlogged anyway.
+ */
+ OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, persistence,
+ lockmode);
- /*
- * Copy the heap data into the new table with the desired
- * modifications, and test the current data within the table
- * against new constraints generated by ALTER TABLE commands.
- */
- ATRewriteTable(tab, OIDNewHeap, lockmode);
+ /*
+ * Copy the heap data into the new table with the desired
+ * modifications, and test the current data within the table
+ * against new constraints generated by ALTER TABLE commands.
+ */
+ ATRewriteTable(tab, OIDNewHeap, lockmode);
- /*
- * Swap the physical files of the old and new heaps, then rebuild
- * indexes and discard the old heap. We can use RecentXmin for
- * the table's new relfrozenxid because we rewrote all the tuples
- * in ATRewriteTable, so no older Xid remains in the table. Also,
- * we never try to swap toast tables by content, since we have no
- * interest in letting this code work on system catalogs.
- */
- finish_heap_swap(tab->relid, OIDNewHeap,
- false, false, true,
- !OidIsValid(tab->newTableSpace),
- RecentXmin,
- ReadNextMultiXactId(),
- persistence);
+ /*
+ * Swap the physical files of the old and new heaps, then
+ * rebuild indexes and discard the old heap. We can use
+ * RecentXmin for the table's new relfrozenxid because we
+ * rewrote all the tuples in ATRewriteTable, so no older Xid
+ * remains in the table. Also, we never try to swap toast
+ * tables by content, since we have no interest in letting this
+ * code work on system catalogs.
+ */
+ finish_heap_swap(tab->relid, OIDNewHeap,
+ false, false, true,
+ !OidIsValid(tab->newTableSpace),
+ RecentXmin,
+ ReadNextMultiXactId(),
+ persistence);
+ }
}
else
{
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index ad0d1a9abc..c71e1a5f92 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3033,6 +3033,80 @@ DropRelFileNodeBuffers(RelFileNodeBackend rnode, ForkNumber *forkNum,
}
}
+/* ---------------------------------------------------------------------
+ * SetRelFileNodeBuffersPersistence
+ *
+ * This function changes the persistence of all buffer pages of a relation
+ * then writes all dirty pages of the relation out to disk when switching
+ * to PERMANENT. (or more accurately, out to kernel disk buffers),
+ * ensuring that the kernel has an up-to-date view of the relation.
+ *
+ * Generally, the caller should be holding AccessExclusiveLock on the
+ * target relation to ensure that no other backend is busy dirtying
+ * more blocks of the relation; the effects can't be expected to last
+ * after the lock is released.
+ *
+ * XXX currently it sequentially searches the buffer pool, should be
+ * changed to more clever ways of searching. This routine is not
+ * used in any performance-critical code paths, so it's not worth
+ * adding additional overhead to normal paths to make it go faster;
+ * but see also DropRelFileNodeBuffers.
+ * --------------------------------------------------------------------
+ */
+void
+SetRelationBuffersPersistence(Relation rel, bool permanent)
+{
+ int i;
+ RelFileNodeBackend rnode = rel->rd_smgr->smgr_rnode;
+
+ Assert (!RelFileNodeBackendIsTemp(rnode));
+
+ for (i = 0; i < NBuffers; i++)
+ {
+ BufferDesc *bufHdr = GetBufferDescriptor(i);
+ uint32 buf_state;
+
+ if (!RelFileNodeEquals(bufHdr->tag.rnode, rnode.node))
+ continue;
+
+ ReservePrivateRefCountEntry();
+
+ buf_state = LockBufHdr(bufHdr);
+
+ if (RelFileNodeEquals(bufHdr->tag.rnode, rnode.node))
+ {
+ ereport(LOG, (errmsg ("#%d: %d", i, (buf_state & BM_PERMANENT) == 0), errhidestmt(true)));
+ if (permanent)
+ {
+ Assert ((buf_state & BM_PERMANENT) == 0);
+ buf_state |= BM_PERMANENT;
+ pg_atomic_write_u32(&bufHdr->state, buf_state);
+
+ /* we flush this buffer when swithing to PERMANENT */
+ if ((buf_state & (BM_VALID | BM_DIRTY)) ==
+ (BM_VALID | BM_DIRTY))
+ {
+ PinBuffer_Locked(bufHdr);
+ LWLockAcquire(BufferDescriptorGetContentLock(bufHdr),
+ LW_SHARED);
+ FlushBuffer(bufHdr, rel->rd_smgr);
+ LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
+ UnpinBuffer(bufHdr, true);
+ }
+ else
+ UnlockBufHdr(bufHdr, buf_state);
+ }
+ else
+ {
+ Assert ((buf_state & BM_PERMANENT) != 0);
+ buf_state &= ~BM_PERMANENT;
+ UnlockBufHdr(bufHdr, buf_state);
+ }
+ ereport(LOG, (errmsg ("#%d: -> %d", i, (buf_state & BM_PERMANENT) == 0), errhidestmt(true)));
+ }
+ }
+}
+
/* ---------------------------------------------------------------------
* DropRelFileNodesAllBuffers
*
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index dcc09df0c7..5eb9e97b3d 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -645,6 +645,12 @@ smgrimmedsync(SMgrRelation reln, ForkNumber forknum)
smgrsw[reln->smgr_which].smgr_immedsync(reln, forknum);
}
+void
+smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo)
+{
+ smgrsw[reln->smgr_which].smgr_unlink(reln->smgr_rnode, forknum, isRedo);
+}
+
/*
* AtEOXact_SMgr
*
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 387eb34a61..1d19278a18 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -451,6 +451,15 @@ typedef struct TableAmRoutine
TransactionId *freezeXid,
MultiXactId *minmulti);
+ /*
+ * This callback needs to switch persistence of the relation between
+ * RELPERSISTENCE_PERMANENT and RELPERSISTENCE_UNLOGGED. Actual change on
+ * storage is performed elsewhere.
+ *
+ * See also table_relation_set_persistence().
+ */
+ void (*relation_set_persistence) (Relation rel, char persistence);
+
/*
* This callback needs to remove all contents from `rel`'s current
* relfilenode. No provisions for transactional behaviour need to be made.
@@ -1404,6 +1413,18 @@ table_relation_set_new_filenode(Relation rel,
freezeXid, minmulti);
}
+/*
+ * Switch storage persistence between RELPERSISTENCE_PERMANENT and
+ * RELPERSISTENCE_UNLOGGED.
+ *
+ * This is used during in-place persistence switching
+ */
+static inline void
+table_relation_set_persistence(Relation rel, char persistence)
+{
+ rel->rd_tableam->relation_set_persistence(rel, persistence);
+}
+
/*
* Remove all table contents from `rel`, in a non-transactional manner.
* Non-transactional meaning that there's no need to support rollbacks. This
diff --git a/src/include/catalog/storage.h b/src/include/catalog/storage.h
index 30c38e0ca6..43d2eb0fb4 100644
--- a/src/include/catalog/storage.h
+++ b/src/include/catalog/storage.h
@@ -23,6 +23,8 @@
extern int wal_skip_threshold;
extern SMgrRelation RelationCreateStorage(RelFileNode rnode, char relpersistence);
+extern void RelationCreateInitFork(RelFileNode rel, bool isRedo);
+extern void RelationDropInitFork(RelFileNode rel);
extern void RelationDropStorage(Relation rel);
extern void RelationPreserveStorage(RelFileNode rnode, bool atCommit);
extern void RelationPreTruncate(Relation rel);
diff --git a/src/include/catalog/storage_xlog.h b/src/include/catalog/storage_xlog.h
index 7b21cab2e0..73ad2ae89e 100644
--- a/src/include/catalog/storage_xlog.h
+++ b/src/include/catalog/storage_xlog.h
@@ -29,6 +29,7 @@
/* XLOG gives us high 4 bits */
#define XLOG_SMGR_CREATE 0x10
#define XLOG_SMGR_TRUNCATE 0x20
+#define XLOG_SMGR_UNLINK 0x30
typedef struct xl_smgr_create
{
@@ -36,6 +37,12 @@ typedef struct xl_smgr_create
ForkNumber forkNum;
} xl_smgr_create;
+typedef struct xl_smgr_unlink
+{
+ RelFileNode rnode;
+ ForkNumber forkNum;
+} xl_smgr_unlink;
+
/* flags for xl_smgr_truncate */
#define SMGR_TRUNCATE_HEAP 0x0001
#define SMGR_TRUNCATE_VM 0x0002
@@ -51,6 +58,7 @@ typedef struct xl_smgr_truncate
} xl_smgr_truncate;
extern void log_smgrcreate(const RelFileNode *rnode, ForkNumber forkNum);
+extern void log_smgrunlink(const RelFileNode *rnode, ForkNumber forkNum);
extern void smgr_redo(XLogReaderState *record);
extern void smgr_desc(StringInfo buf, XLogReaderState *record);
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index ee91b8fa26..f65a273999 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -205,6 +205,7 @@ extern void FlushRelationsAllBuffers(struct SMgrRelationData **smgrs, int nrels)
extern void FlushDatabaseBuffers(Oid dbid);
extern void DropRelFileNodeBuffers(RelFileNodeBackend rnode, ForkNumber *forkNum,
int nforks, BlockNumber *firstDelBlock);
+extern void SetRelationBuffersPersistence(Relation rnode, bool permanent);
extern void DropRelFileNodesAllBuffers(RelFileNodeBackend *rnodes, int nnodes);
extern void DropDatabaseBuffers(Oid dbid);
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index f28a842401..5d74631006 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -86,6 +86,7 @@ extern void smgrclose(SMgrRelation reln);
extern void smgrcloseall(void);
extern void smgrclosenode(RelFileNodeBackend rnode);
extern void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
+extern void smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo);
extern void smgrdosyncall(SMgrRelation *rels, int nrels);
extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum,
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: In-place persistance change of a relation
@ 2020-11-11 14:56 Stephen Frost <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
2 siblings, 1 reply; 10+ messages in thread
From: Stephen Frost @ 2020-11-11 14:56 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Greetings,
* Kyotaro Horiguchi ([email protected]) wrote:
> At Tue, 10 Nov 2020 09:33:12 -0500, Stephen Frost <[email protected]> wrote in
> > * Kyotaro Horiguchi ([email protected]) wrote:
> > > For fuel(?) of the discussion, I tried a very-quick PoC for in-place
> > > ALTER TABLE SET LOGGED/UNLOGGED and resulted as attached. After some
> > > trials of several ways, I drifted to the following way after poking
> > > several ways.
> > >
> > > 1. Flip BM_PERMANENT of active buffers
> > > 2. adding/removing init fork
> > > 3. sync files,
> > > 4. Flip pg_class.relpersistence.
> > >
> > > It always skips table copy in the SET UNLOGGED case, and only when
> > > wal_level=minimal in the SET LOGGED case. Crash recovery seems
> > > working by some brief testing by hand.
> >
> > Somehow missed that this patch more-or-less does what I was referring to
> > down-thread, but I did want to mention that it looks like it's missing a
> > necessary FlushRelationBuffers() call before the sync, otherwise there
> > could be dirty buffers for the relation that's being set to LOGGED (with
> > wal_level=minimal), which wouldn't be good. See the comments above
> > smgrimmedsync().
>
> Right. Thanks. However, since SetRelFileNodeBuffersPersistence()
> called just above scans shared buffers so I don't want to just call
> FlushRelationBuffers() separately. Instead, I added buffer-flush to
> SetRelFileNodeBuffersPersistence().
Maybe I'm missing something, but it sure looks like in the patch that
SetRelFileNodeBuffersPersistence() is being called after the
smgrimmedsync() call, and I don't think you get to just switch the order
of those- the sync is telling the kernel to make sure it's written to
disk, while the FlushBuffer() is just writing it into the kernel but
doesn't provide any guarantee that the data has actually made it to
disk. We have to FlushBuffer() first, and then call smgrimmedsync().
Perhaps there's a way to avoid having to go through shared buffers
twice, and I generally agreed it'd be good if we could avoid doing so,
but this approach doesn't look like it actually works.
> FWIW this is a revised version of the PoC, which has some known
> problems.
>
> - Flipping of Buffer persistence is not WAL-logged nor even be able to
> be safely roll-backed. (It might be better to drop buffers).
Not sure if it'd be better to drop buffers or not, but figuring out how
to deal with rollback seems pretty important. How is the persistence
change in the catalog not WAL-logged though..?
> - This version handles indexes but not yet handle toast relatins.
Would need to be fixed, of course.
> - tableAMs are supposed to support this feature. (but I'm not sure
> it's worth allowing them not to do so).
Seems like they should.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-11 22:18 Andres Freund <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
2 siblings, 1 reply; 10+ messages in thread
From: Andres Freund @ 2020-11-11 22:18 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi,
I suggest outlining what you are trying to achieve here. Starting a new
thread and expecting people to dig through another thread to infer what
you are actually trying to achive isn't great.
FWIW, I'm *extremely* doubtful it's worth adding features that depend on
a PGC_POSTMASTER wal_level=minimal being used. Which this does, a far as
I understand. If somebody added support for dynamically adapting
wal_level (e.g. wal_level=auto, that increases wal_level to
replica/logical depending on the presence of replication slots), it'd
perhaps be different.
On 2020-11-11 17:33:17 +0900, Kyotaro Horiguchi wrote:
> FWIW this is a revised version of the PoC, which has some known
> problems.
>
> - Flipping of Buffer persistence is not WAL-logged nor even be able to
> be safely roll-backed. (It might be better to drop buffers).
That's obviously a no-go. I think you might be able to address this if
you accept that the command cannot be run in a transaction (like
CONCURRENTLY). Then you can first do the catalog changes, change the
persistence level, and commit.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-12 06:55 Kyotaro Horiguchi <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-12 06:55 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Wed, 11 Nov 2020 14:18:04 -0800, Andres Freund <[email protected]> wrote in
> Hi,
>
> I suggest outlining what you are trying to achieve here. Starting a new
> thread and expecting people to dig through another thread to infer what
> you are actually trying to achive isn't great.
Agreed. I'll post that. Thanks.
> FWIW, I'm *extremely* doubtful it's worth adding features that depend on
> a PGC_POSTMASTER wal_level=minimal being used. Which this does, a far as
> I understand. If somebody added support for dynamically adapting
> wal_level (e.g. wal_level=auto, that increases wal_level to
> replica/logical depending on the presence of replication slots), it'd
> perhaps be different.
Yes, this depends on wal_level=minimal for switching from UNLOGGED to
LOGGED, that's similar to COPY/INSERT-to-intransaction-created-tables
optimization for wal_level=minimal. And it expands that optimization
to COPY/INSERT-to-existent-tables, which seems worth doing.
Switching to LOGGED needs to emit the initial state to WAL... Hmm.. I
came to think that even in that case skipping table copy reduces I/O
significantly, even though FPI-WAL is emitted.
> On 2020-11-11 17:33:17 +0900, Kyotaro Horiguchi wrote:
> > FWIW this is a revised version of the PoC, which has some known
> > problems.
> >
> > - Flipping of Buffer persistence is not WAL-logged nor even be able to
> > be safely roll-backed. (It might be better to drop buffers).
>
> That's obviously a no-go. I think you might be able to address this if
> you accept that the command cannot be run in a transaction (like
> CONCURRENTLY). Then you can first do the catalog changes, change the
> persistence level, and commit.
Of course. The next version reverts persistence change at abort.
Thanks!
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: In-place persistance change of a relation
@ 2020-11-12 06:55 Kyotaro Horiguchi <[email protected]>
parent: Stephen Frost <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-12 06:55 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Wed, 11 Nov 2020 09:56:44 -0500, Stephen Frost <[email protected]> wrote in
> Greetings,
>
> * Kyotaro Horiguchi ([email protected]) wrote:
> > At Tue, 10 Nov 2020 09:33:12 -0500, Stephen Frost <[email protected]> wrote in
> > > * Kyotaro Horiguchi ([email protected]) wrote:
> > > > For fuel(?) of the discussion, I tried a very-quick PoC for in-place
> > > > ALTER TABLE SET LOGGED/UNLOGGED and resulted as attached. After some
> > > > trials of several ways, I drifted to the following way after poking
> > > > several ways.
> > > >
> > > > 1. Flip BM_PERMANENT of active buffers
> > > > 2. adding/removing init fork
> > > > 3. sync files,
> > > > 4. Flip pg_class.relpersistence.
> > > >
> > > > It always skips table copy in the SET UNLOGGED case, and only when
> > > > wal_level=minimal in the SET LOGGED case. Crash recovery seems
> > > > working by some brief testing by hand.
> > >
> > > Somehow missed that this patch more-or-less does what I was referring to
> > > down-thread, but I did want to mention that it looks like it's missing a
> > > necessary FlushRelationBuffers() call before the sync, otherwise there
> > > could be dirty buffers for the relation that's being set to LOGGED (with
> > > wal_level=minimal), which wouldn't be good. See the comments above
> > > smgrimmedsync().
> >
> > Right. Thanks. However, since SetRelFileNodeBuffersPersistence()
> > called just above scans shared buffers so I don't want to just call
> > FlushRelationBuffers() separately. Instead, I added buffer-flush to
> > SetRelFileNodeBuffersPersistence().
>
> Maybe I'm missing something, but it sure looks like in the patch that
> SetRelFileNodeBuffersPersistence() is being called after the
> smgrimmedsync() call, and I don't think you get to just switch the order
> of those- the sync is telling the kernel to make sure it's written to
> disk, while the FlushBuffer() is just writing it into the kernel but
> doesn't provide any guarantee that the data has actually made it to
> disk. We have to FlushBuffer() first, and then call smgrimmedsync().
> Perhaps there's a way to avoid having to go through shared buffers
> twice, and I generally agreed it'd be good if we could avoid doing so,
> but this approach doesn't look like it actually works.
Yeah, sorry for the rare-baked version.. I was confused about the
order at the time. The next version works like this:
LOGGED->UNLOGGED
<collect reloids to process>
for each relations:
<set buffer persistence to !BM_PERMANENT (wal-logged if walleve > minimal>
<create init fork>
if it is index call ambuildempty() (which syncs the init fork)
else WAL-log smgr_create then sync the init file.
<update catalog>
...
commit time:
<do nogthing>
abort time:
<unlink init fork>
<revert buffer persistence>
UNLOGGED->LOGGED
<collect reloids to process>
for each relations:
<set buffer persistence to !BM_PERMANENT (wal-logged if walleve > minimal>
<record drop-init-fork to pending-deletes>
<sync storage files>
<update catalog>
...
commit time:
<log smgrunlink>
<smgrunlink init fork>
abort time:
<revert buffer persistence>
> > FWIW this is a revised version of the PoC, which has some known
> > problems.
> >
> > - Flipping of Buffer persistence is not WAL-logged nor even be able to
> > be safely roll-backed. (It might be better to drop buffers).
>
> Not sure if it'd be better to drop buffers or not, but figuring out how
> to deal with rollback seems pretty important. How is the persistence
> change in the catalog not WAL-logged though..?
Rollback works as the above. Buffer persistence change is registered
in pending-deletes. Persistence change in catalog is rolled back in
the ordinary way (or automatically).
If wal_level > minimal, persistence change of buffers is propagated to
standbys by WAL. However I'm not sure we need wal-logging otherwise,
the next version emits WAL since SMGR_CREATE is always logged by
existing code.
> > - This version handles indexes but not yet handle toast relatins.
>
> Would need to be fixed, of course.
Fixed.
> > - tableAMs are supposed to support this feature. (but I'm not sure
> > it's worth allowing them not to do so).
>
> Seems like they should.
Init fork of index relations needs a call to ambuildempty() instead of
"log_smgrcreate-smgrimmedsync" after smgrcreate. Instead of adding
similar interface in indexAm, I reverted changes of tableam and make
RelationCreate/DropInitFork() directly do that. That introduces new
include of amapi.h to storage.c, which is a bit uneasy.
The previous version give up the in-place persistence change in the
case where wal_level > minimal and SET LOGGED since that needs WAL to
be emitted. However, in-place change still has the advantage of not
running a table copy. So the next verson always runs persistence
change in-place.
As suggested by Andres, I'll send a summary of this patch. The patch
will be attached to the coming mail.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-13 04:22 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
2 siblings, 1 reply; 10+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-13 04:22 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hello. Before posting the next version, I'd like to explain what this
patch is.
1. The Issue
Bulk data loading is a long-time taking, I/O consuming task. Many
DBAs want that task is faster, even at the cost of increasing risk of
data-loss. wal_level=minimal is an answer to such a
request. Data-loading onto a table that is created in the current
transaction omits WAL-logging and synced at commit.
However, the optimization doesn't benefit the case where the
data-loading is performed onto existing tables. There are quite a few
cases where data is loaded into tables that already contains a lot of
data. Those cases don't take benefit of the optimization.
Another possible solution for bulk data-loading is UNLOGGED
tables. But when we switch LOGGED/UNLOGGED of a table, all the table
content is copied to a newly created heap, which is costly.
2. Proposed Solutions.
There are two proposed solutions are discussed on this mailing
list. One is wal_level = none (*1), which omits WAL-logging almost at
all. Another is extending the existing optimization to the ALTER TABLE
SET LOGGED/UNLOGGED cases, which is to be discussed in this new
thread.
3. In-place Persistence Change
So the attached is a PoC patch of the "another" solution. When we
want to change table persistence in-place, basically we need to do the
following steps.
(the talbe is exclusively locked)
(1) Flip BM_PERMANENT flag of all shared buffer blocks for the heap.
(2) Create or delete the init fork for existing heap.
(3) Flush all buffers of the relation to file system.
(4) Sync heap files.
(5) Make catalog changes.
4. Transactionality
The 1, 2 and 5 above need to be abort-able. 5 is rolled back by
existing infrastructure, and rolling-back of 1 and 2 are achieved by
piggybacking on the pendingDeletes mechanism.
5. Replication
Furthermore, that changes ought to be replicable to standbys. Catalog
changes are replicated as usual.
On-the-fly creation of the init fork leads to recovery mess. Even
though it is removed at abort, if the server crashed before
transaction end, the file is left alone and corrupts database in the
next recovery. I sought a way to create the init fork in
smgrPendingDelete but that needs relcache and relcache is not
available at that late of commit. Finally, I introduced the fifth fork
kind "INITTMP"(_itmp) only to signal that the init file is not
committed. I don't like that way but it seems working fine...
6. SQL Command
The second file in the patchset adds a syntax that changes persistence
of all tables in a tablespace.
ALTER TABLE ALL IN TABLESPACE <tsp> SET LOGGED/UNLOGGED [ NOWAIT ];
7. Testing
I tried to write TAP test for this, but IPC::Run::harness (or
interactive_psql) doesn't seem to work for me. I'm not sure what
exactly is happening but pty redirection doesn't work.
$in = "ls\n"; $out = ""; run ["/usr/bin/bash"], \$in, \$out; print $out;
works but
$in = "ls\n"; $out = ""; run ["/usr/bin/bash"], '<pty<', \$in, '>pty>', \$out; print $out;
doesn't respond.
The patch is attached.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 10+ messages in thread
* RE: In-placre persistance change of a relation
@ 2020-11-13 06:43 [email protected] <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 2 replies; 10+ messages in thread
From: [email protected] @ 2020-11-13 06:43 UTC (permalink / raw)
To: 'Kyotaro Horiguchi' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
Hi Horiguchi-san,
Thank you for making a patch so quickly. I've started looking at it.
What makes you think this is a PoC? Documentation and test cases? If there's something you think that doesn't work or are concerned about, can you share it?
Do you know the reason why data copy was done before? And, it may be odd for me to ask this, but I think I saw someone referred to the past discussion that eliminating data copy is difficult due to some processing at commit. I can't find it.
(1)
@@ -168,6 +168,8 @@ extern PGDLLIMPORT int32 *LocalRefCount;
*/
#define BufferGetPage(buffer) ((Page)BufferGetBlock(buffer))
+struct SmgrRelationData;
This declaration is already in the file:
/* forward declared, to avoid having to expose buf_internals.h here */
struct WritebackContext;
/* forward declared, to avoid including smgr.h here */
struct SMgrRelationData;
Regards
Takayuki Tsunakawa
^ permalink raw reply [nested|flat] 10+ messages in thread
* RE: In-placre persistance change of a relation
@ 2020-11-13 07:15 [email protected] <[email protected]>
parent: [email protected] <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: [email protected] @ 2020-11-13 07:15 UTC (permalink / raw)
To: [email protected] <[email protected]>; 'Kyotaro Horiguchi' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
Hello, Tsunakawa-San
> Do you know the reason why data copy was done before? And, it may be
> odd for me to ask this, but I think I saw someone referred to the past
> discussion that eliminating data copy is difficult due to some processing at
> commit. I can't find it.
I can share 2 sources why to eliminate the data copy is difficult in hackers thread.
Tom's remark and the context to copy relation's data.
https://www.postgresql.org/message-id/flat/31724.1394163360%40sss.pgh.pa.us#[email protected]...
Amit-San quoted this thread and mentioned that point in another thread.
https://www.postgresql.org/message-id/CAA4eK1%2BHDqS%2B1fhs5Jf9o4ZujQT%3DXBZ6sU0kOuEh2hqQAC%2Bt%3Dw%...
Best,
Takamichi Osumi
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-13 07:47 Kyotaro Horiguchi <[email protected]>
parent: [email protected] <[email protected]>
1 sibling, 0 replies; 10+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-13 07:47 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 13 Nov 2020 06:43:13 +0000, "[email protected]" <[email protected]> wrote in
> Hi Horiguchi-san,
>
>
> Thank you for making a patch so quickly. I've started looking at it.
>
> What makes you think this is a PoC? Documentation and test cases? If there's something you think that doesn't work or are concerned about, can you share it?
The latest version is heavily revised and is given much comment so it
might have exited from PoC state. The necessity of documentation is
doubtful since this patch doesn't user-facing behavior other than
speed. Some tests are required especialy about recovery and
replication perspective but I haven't been able to make it. (One of
the tests needs to cause crash while a transaction is running.)
> Do you know the reason why data copy was done before? And, it may be odd for me to ask this, but I think I saw someone referred to the past discussion that eliminating data copy is difficult due to some processing at commit. I can't find it.
To imagine that, just because it is simpler considering rollback and
code sharing, and maybe no one have been complained that SET
LOGGED/UNLOGGED looks taking a long time than required/expected.
The current implement is simple. It's enough to just discard old or
new relfilenode according to the current transaction ends with commit
or abort. Tweaking of relfilenode under use leads-in some skews in
some places. I used pendingDelete mechanism a bit complexified way
and a violated an abstraction (I think, calling AM-routines from
storage.c is not good.) and even introduce a new fork kind only to
mark a init fork as "not committed yet". There might be better way,
but I haven't find it.
(The patch scans all shared buffer blocks for each relation).
> (1)
> @@ -168,6 +168,8 @@ extern PGDLLIMPORT int32 *LocalRefCount;
> */
> #define BufferGetPage(buffer) ((Page)BufferGetBlock(buffer))
>
> +struct SmgrRelationData;
>
> This declaration is already in the file:
>
> /* forward declared, to avoid having to expose buf_internals.h here */
> struct WritebackContext;
>
> /* forward declared, to avoid including smgr.h here */
> struct SMgrRelationData;
Hmmm. Nice chatch. And will fix in the next version.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-13 08:23 Kyotaro Horiguchi <[email protected]>
parent: [email protected] <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-13 08:23 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 13 Nov 2020 07:15:41 +0000, "[email protected]" <[email protected]> wrote in
> Hello, Tsunakawa-San
>
Thanks for sharing it!
> > Do you know the reason why data copy was done before? And, it may be
> > odd for me to ask this, but I think I saw someone referred to the past
> > discussion that eliminating data copy is difficult due to some processing at
> > commit. I can't find it.
> I can share 2 sources why to eliminate the data copy is difficult in hackers thread.
>
> Tom's remark and the context to copy relation's data.
> https://www.postgresql.org/message-id/flat/31724.1394163360%40sss.pgh.pa.us#[email protected]...
https://www.postgresql.org/message-id/[email protected]...
> No, not really. The issue is more around what happens if we crash
> part way through. At crash recovery time, the system catalogs are not
> available, because the database isn't consistent yet and, anyway, the
> startup process can't be bound to a database, let alone every database
> that might contain unlogged tables. So the sentinel that's used to
> decide whether to flush the contents of a table or index is the
> presence or absence of an _init fork, which the startup process
> obviously can see just fine. The _init fork also tells us what to
> stick in the relation when we reset it; for a table, we can just reset
> to an empty file, but that's not legal for indexes, so the _init fork
> contains a pre-initialized empty index that we can just copy over.
>
> Now, to make an unlogged table logged, you've got to at some stage
> remove those _init forks. But this is not a transactional operation.
> If you remove the _init forks and then the transaction rolls back,
> you've left the system an inconsistent state. If you postpone the
> removal until commit time, then you have a problem if it fails,
It's true. That are the cause of headache.
> particularly if it works for the first file but fails for the second.
> And if you crash at any point before you've fsync'd the containing
> directory, you have no idea which files will still be on disk after a
> hard reboot.
This is not an issue in this patch *except* the case where init fork
is failed to removed but the following removal of inittmp fork
succeeds. Another idea is adding a "not-yet-committed" property to a
fork. I added a new fork type for easiness of the patch but I could
go that way if that is an issue.
> Amit-San quoted this thread and mentioned that point in another thread.
> https://www.postgresql.org/message-id/CAA4eK1%2BHDqS%2B1fhs5Jf9o4ZujQT%3DXBZ6sU0kOuEh2hqQAC%2Bt%3Dw%...
This sounds like a bit differrent discussion. Making part-of-a-table
UNLOGGED looks far difficult to me.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2020-11-13 08:23 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-11 08:33 In-placre persistance change of a relation Kyotaro Horiguchi <[email protected]>
2020-11-11 14:56 ` Re: In-place persistance change of a relation Stephen Frost <[email protected]>
2020-11-12 06:55 ` Re: In-place persistance change of a relation Kyotaro Horiguchi <[email protected]>
2020-11-11 22:18 ` Andres Freund <[email protected]>
2020-11-12 06:55 ` Kyotaro Horiguchi <[email protected]>
2020-11-13 04:22 ` Kyotaro Horiguchi <[email protected]>
2020-11-13 06:43 ` [email protected] <[email protected]>
2020-11-13 07:15 ` [email protected] <[email protected]>
2020-11-13 08:23 ` Kyotaro Horiguchi <[email protected]>
2020-11-13 07:47 ` Kyotaro Horiguchi <[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