public inbox for [email protected]help / color / mirror / Atom feed
Re: eclg -C ORACLE breaks data 6+ messages / 3 participants [nested] [flat]
* Re: eclg -C ORACLE breaks data @ 2023-04-17 08:00 Michael Paquier <[email protected]> 0 siblings, 2 replies; 6+ messages in thread From: Michael Paquier @ 2023-04-17 08:00 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] On Mon, Apr 10, 2023 at 05:35:00PM +0900, Kyotaro Horiguchi wrote: > This results in overwriting str[-1], the last byte of the preceding > numeric in this case, with 0x00, representing the digit '0'. When > callers of ecpg_get_data() explicitly pass zero as varcharsize, they > provide storage that precisely fitting the data. Good find, that's clearly wrong. The test case is interesting. On HEAD, the processing of the second field eats up the data of the first field. > However, it remains > uncertain if this assumption is valid when ecpg_store_result() passes > var->varcharsize which is also zero. Consequently, the current fix > presumes exact-fit storage when varcharsize is zero. Based on what I can read in sqlda.c (ecpg_set_compat_sqlda() and ecpg_set_native_sqlda()), the data length calculated adds an extra byte to the data length when storing the data references in sqldata. execute.c and ecpg_store_result() is actually much trickier than that (see particularly the part where the code does an "allocate memory for NULL pointers", where varcharsize could also be 0), still I agree that this assumption should be OK. The code is as it is for many years, with its logic to do an estimation of allocation first, and then read the data at once in the whole area allocated.. This thinko has been introduced by 3b7ab43, so this needs to go down to v11. I'll see to that. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: eclg -C ORACLE breaks data @ 2023-04-17 08:47 Kyotaro Horiguchi <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 0 replies; 6+ messages in thread From: Kyotaro Horiguchi @ 2023-04-17 08:47 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] (sorry for the wrong subject..) At Mon, 17 Apr 2023 17:00:59 +0900, Michael Paquier <[email protected]> wrote in > On Mon, Apr 10, 2023 at 05:35:00PM +0900, Kyotaro Horiguchi wrote: > > This results in overwriting str[-1], the last byte of the preceding > > numeric in this case, with 0x00, representing the digit '0'. When > > callers of ecpg_get_data() explicitly pass zero as varcharsize, they > > provide storage that precisely fitting the data. > > Good find, that's clearly wrong. The test case is interesting. On > HEAD, the processing of the second field eats up the data of the first > field. > > > However, it remains > > uncertain if this assumption is valid when ecpg_store_result() passes > > var->varcharsize which is also zero. Consequently, the current fix > > presumes exact-fit storage when varcharsize is zero. > > Based on what I can read in sqlda.c (ecpg_set_compat_sqlda() and > ecpg_set_native_sqlda()), the data length calculated adds an extra > byte to the data length when storing the data references in sqldata. > execute.c and ecpg_store_result() is actually much trickier than that > (see particularly the part where the code does an "allocate memory for > NULL pointers", where varcharsize could also be 0), still I agree that > this assumption should be OK. The code is as it is for many years, > with its logic to do an estimation of allocation first, and then read > the data at once in the whole area allocated.. > > This thinko has been introduced by 3b7ab43, so this needs to go down > to v11. I'll see to that. Thanks for picking this up. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: eclg -C ORACLE breaks data @ 2023-04-18 02:35 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 1 reply; 6+ messages in thread From: Michael Paquier @ 2023-04-18 02:35 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] On Mon, Apr 17, 2023 at 05:00:59PM +0900, Michael Paquier wrote: > This thinko has been introduced by 3b7ab43, so this needs to go down > to v11. I'll see to that. So done. mylodon is feeling unhappy about that, because this has a C99 declaration: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mylodon&dt=2023-04-18%2002%3A22%3A04 char_array.pgc:73:8: error: variable declaration in for loop is a C99-specific feature [-Werror,-Wc99-extensions] for (int i = 0 ; i < sqlda->sqld ; i++) I'll go fix that in a minute, across all the branches for consistency. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: eclg -C ORACLE breaks data @ 2023-04-20 03:56 Kyotaro Horiguchi <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Kyotaro Horiguchi @ 2023-04-20 03:56 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] At Tue, 18 Apr 2023 11:35:16 +0900, Michael Paquier <[email protected]> wrote in > On Mon, Apr 17, 2023 at 05:00:59PM +0900, Michael Paquier wrote: > > This thinko has been introduced by 3b7ab43, so this needs to go down > > to v11. I'll see to that. > > So done. mylodon is feeling unhappy about that, because this has a Thanks! > C99 declaration: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mylodon&dt=2023-04-18%2002%3A22%3A04 > char_array.pgc:73:8: error: variable declaration in for loop is a C99-specific feature [-Werror,-Wc99-extensions] > for (int i = 0 ; i < sqlda->sqld ; i++) > > I'll go fix that in a minute, across all the branches for > consistency. Oh, I didn't realize there were differences in the configurations. Good to know. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: eclg -C ORACLE breaks data @ 2023-04-20 04:00 Michael Paquier <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Michael Paquier @ 2023-04-20 04:00 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] On Thu, Apr 20, 2023 at 12:56:32PM +0900, Kyotaro Horiguchi wrote: > Oh, I didn't realize there were differences in the > configurations. Good to know. C99 declarations are OK in v12~, so with v11 going out of sight in approximately 6 month, it won't matter soon ;) -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../ZEC49DmJ%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 6+ messages in thread
* [PATCH 3/5] Move conversion of a "historic" to MVCC snapshot to a separate function. @ 2025-12-09 18:44 Antonin Houska <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Antonin Houska @ 2025-12-09 18:44 UTC (permalink / raw) The conversion is now handled by SnapBuildMVCCFromHistoric(). REPACK CONCURRENTLY will also need it. --- src/backend/commands/cluster.c | 8 ++- src/backend/replication/logical/snapbuild.c | 57 +++++++++++++++++---- src/backend/utils/time/snapmgr.c | 3 +- src/include/replication/snapbuild.h | 1 + src/include/utils/snapmgr.h | 1 + 5 files changed, 53 insertions(+), 17 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 3afab656cd9..89f0b03a31c 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -70,8 +70,7 @@ typedef struct static bool cluster_rel_recheck(RepackCommand cmd, Relation OldHeap, Oid indexOid, Oid userid, int options); -static void rebuild_relation(RepackCommand cmd, - Relation OldHeap, Relation index, bool verbose); +static void rebuild_relation(Relation OldHeap, Relation index, bool verbose); static void copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, bool verbose, bool *pSwapToastByContent, TransactionId *pFreezeXid, MultiXactId *pCutoffMulti); @@ -415,7 +414,7 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid, TransferPredicateLocksToHeapRelation(OldHeap); /* rebuild_relation does all the dirty work */ - rebuild_relation(cmd, OldHeap, index, verbose); + rebuild_relation(OldHeap, index, verbose); /* rebuild_relation closes OldHeap, and index if valid */ out: @@ -629,8 +628,7 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal) * On exit, they are closed, but locks on them are not released. */ static void -rebuild_relation(RepackCommand cmd, - Relation OldHeap, Relation index, bool verbose) +rebuild_relation(Relation OldHeap, Relation index, bool verbose) { Oid tableOid = RelationGetRelid(OldHeap); Oid accessMethod = OldHeap->rd_rel->relam; diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index 6e18baa33cb..34bdd987478 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -440,10 +440,7 @@ Snapshot SnapBuildInitialSnapshot(SnapBuild *builder) { Snapshot snap; - TransactionId xid; TransactionId safeXid; - TransactionId *newxip; - int newxcnt = 0; Assert(XactIsoLevel == XACT_REPEATABLE_READ); Assert(builder->building_full_snapshot); @@ -485,7 +482,33 @@ SnapBuildInitialSnapshot(SnapBuild *builder) MyProc->xmin = snap->xmin; - /* allocate in transaction context */ + /* Convert the historic snapshot to MVCC snapshot. */ + return SnapBuildMVCCFromHistoric(snap, true); +} + +/* + * Turn a historic MVCC snapshot into an ordinary MVCC snapshot. + * + * Unlike a regular (non-historic) MVCC snapshot, the 'xip' array of this + * snapshot contains not only running main transactions, but also their + * subtransactions. On the other hand, 'subxip' will usually be empty. This + * difference does not affect the result of XidInMVCCSnapshot() because it + * searches both in 'xip' and 'subxip'. + * + * Pass true for 'in_place' if you don't care about modifying the source + * snapshot. If you need a new instance, and one that was allocated as a + * single chunk of memory, pass false. + */ +Snapshot +SnapBuildMVCCFromHistoric(Snapshot snapshot, bool in_place) +{ + TransactionId xid; + TransactionId *oldxip = snapshot->xip; + uint32 oldxcnt = snapshot->xcnt; + TransactionId *newxip; + int newxcnt = 0; + Snapshot result; + newxip = (TransactionId *) palloc(sizeof(TransactionId) * GetMaxSnapshotXidCount()); @@ -495,7 +518,7 @@ SnapBuildInitialSnapshot(SnapBuild *builder) * classical snapshot by marking all non-committed transactions as * in-progress. This can be expensive. */ - for (xid = snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax);) + for (xid = snapshot->xmin; NormalTransactionIdPrecedes(xid, snapshot->xmax);) { void *test; @@ -503,7 +526,7 @@ SnapBuildInitialSnapshot(SnapBuild *builder) * Check whether transaction committed using the decoding snapshot * meaning of ->xip. */ - test = bsearch(&xid, snap->xip, snap->xcnt, + test = bsearch(&xid, snapshot->xip, snapshot->xcnt, sizeof(TransactionId), xidComparator); if (test == NULL) @@ -520,11 +543,25 @@ SnapBuildInitialSnapshot(SnapBuild *builder) } /* adjust remaining snapshot fields as needed */ - snap->snapshot_type = SNAPSHOT_MVCC; - snap->xcnt = newxcnt; - snap->xip = newxip; + snapshot->xcnt = newxcnt; + snapshot->xip = newxip; + + if (in_place) + result = snapshot; + else + { + result = CopySnapshot(snapshot); + + /* Restore the original values so the source is intact. */ + snapshot->xip = oldxip; + snapshot->xcnt = oldxcnt; + + /* newxip has been copied */ + pfree(newxip); + } + result->snapshot_type = SNAPSHOT_MVCC; - return snap; + return result; } /* diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 24f73a49d27..886060305f5 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -213,7 +213,6 @@ typedef struct ExportedSnapshot static List *exportedSnapshots = NIL; /* Prototypes for local functions */ -static Snapshot CopySnapshot(Snapshot snapshot); static void UnregisterSnapshotNoOwner(Snapshot snapshot); static void FreeSnapshot(Snapshot snapshot); static void SnapshotResetXmin(void); @@ -604,7 +603,7 @@ SetTransactionSnapshot(Snapshot sourcesnap, VirtualTransactionId *sourcevxid, * The copy is palloc'd in TopTransactionContext and has initial refcounts set * to 0. The returned snapshot has the copied flag set. */ -static Snapshot +Snapshot CopySnapshot(Snapshot snapshot) { Snapshot newsnap; diff --git a/src/include/replication/snapbuild.h b/src/include/replication/snapbuild.h index 44031dcf6e3..6d4d2d1814c 100644 --- a/src/include/replication/snapbuild.h +++ b/src/include/replication/snapbuild.h @@ -73,6 +73,7 @@ extern void FreeSnapshotBuilder(SnapBuild *builder); extern void SnapBuildSnapDecRefcount(Snapshot snap); extern Snapshot SnapBuildInitialSnapshot(SnapBuild *builder); +extern Snapshot SnapBuildMVCCFromHistoric(Snapshot snapshot, bool in_place); extern const char *SnapBuildExportSnapshot(SnapBuild *builder); extern void SnapBuildClearExportedSnapshot(void); extern void SnapBuildResetExportedSnapshotState(void); diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h index 604c1f90216..f65f83c85cd 100644 --- a/src/include/utils/snapmgr.h +++ b/src/include/utils/snapmgr.h @@ -63,6 +63,7 @@ extern Snapshot GetTransactionSnapshot(void); extern Snapshot GetLatestSnapshot(void); extern void SnapshotSetCommandId(CommandId curcid); +extern Snapshot CopySnapshot(Snapshot snapshot); extern Snapshot GetCatalogSnapshot(Oid relid); extern Snapshot GetNonHistoricCatalogSnapshot(Oid relid); extern void InvalidateCatalogSnapshot(void); -- 2.47.3 --=-=-= Content-Type: text/plain Content-Disposition: attachment; filename=v27-0004-Add-CONCURRENTLY-option-to-REPACK-command.patch ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2025-12-09 18:44 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-04-17 08:00 Re: eclg -C ORACLE breaks data Michael Paquier <[email protected]> 2023-04-17 08:47 ` Kyotaro Horiguchi <[email protected]> 2023-04-18 02:35 ` Michael Paquier <[email protected]> 2023-04-20 03:56 ` Kyotaro Horiguchi <[email protected]> 2023-04-20 04:00 ` Michael Paquier <[email protected]> 2025-12-09 18:44 [PATCH 3/5] Move conversion of a "historic" to MVCC snapshot to a separate function. 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