Skip to content

Commit

Permalink
[pg15] fix: move ybctid out of ItemPointer
Browse files Browse the repository at this point in the history
Summary:
The initial merge commit (55782d5) moved ybctid into ItemPointer struct. While there are some merits to it, like common APIs for YB and PG, it comes with a bunch of issues:

- Before this change, PG type 'tid' corresponded to ItemPointer struct. This is no longer true. As a result, the tests PgTypeTest.YbTypeDetailsMatch and TestPgEstimatedDocdbResultWidth#testDocdbResultWidthEstimationArrays fail.
- More work is required to get this integration right. For instance, the current definition of macro `ItemPointerIsValid` is problematic. It returns true as long as at least one of pg/yb fields is set without taking into account whether the row (that item pointer points to) belongs to a YB relation or not.
- When upgrading the pre-existing pg11 based YB clusters (that do not have ybctid inside ItemPointer), the following error will be hit:
   `FATAL:  database files are incompatible with server
   DETAIL:  The database cluster was initialized with TOAST_MAX_CHUNK_SIZE 1996, but the server was compiled with TOAST_MAX_CHUNK_SIZE 1988.`

While all the above issues are solvable, putting ybctid inside ItemPointer is an orthogonal decision to the project-pg15 effort. Hence, revert this change by moving ybctid out. YB master branch has the same behaviour.

When this revision reaches the existing pg15 clusters, they will hit the following issue:
   FATAL:  database files are incompatible with server
   DETAIL:  The database cluster was initialized with TOAST_MAX_CHUNK_SIZE 1988, but the server was compiled with TOAST_MAX_CHUNK_SIZE 1996.

Since we have limited pg15 clusters as of today, and all those are either dev or test, it is best to destroy these and create fresh ones.

Test Plan:
Jenkins: rebase: pg15

for _ in {1..50}; do grep -E 'YbTypeDetailsMatch|TestPgEstimatedDocdbResultWidth' pg15_tests/passing_tests.tsv; done | pg15_tests/run_tests.sh

Reviewers: jason, tfoucher, amartsinchyk

Reviewed By: amartsinchyk

Subscribers: tfoucher, jason, yql

Differential Revision: https://phorge.dev.yugabyte.com/D36731
  • Loading branch information
arpang committed Aug 2, 2024
1 parent da70470 commit b263910
Show file tree
Hide file tree
Showing 38 changed files with 235 additions and 208 deletions.
2 changes: 2 additions & 0 deletions pg15_tests/passing_tests.tsv
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ JAVA org.yb.pgsql.TestPgDepend#testForSuperfluousEntries
JAVA org.yb.pgsql.TestPgDepend#testSequenceDeletionWithCascade
JAVA org.yb.pgsql.TestPgDepend#testTableWithSequenceDeletion
JAVA org.yb.pgsql.TestPgDepend#testViewDeletionWithCascade
JAVA org.yb.pgsql.TestPgEstimatedDocdbResultWidth#testDocdbResultWidthEstimationArrays
JAVA org.yb.pgsql.TestPgExplainAnalyze
JAVA org.yb.pgsql.TestPgExplainAnalyzeColocated
JAVA org.yb.pgsql.TestPgExplainAnalyzeMetrics
Expand Down Expand Up @@ -556,6 +557,7 @@ pgwrapper_pg_tablet_split-test PgTabletSplitTest/PgPartitioningTest.PgGatePartit
pgwrapper_pg_tablet_split-test PgTabletSplitTest/PgPartitioningVersionTest.ManualSplit/0
pgwrapper_pg_tablet_split-test PgTabletSplitTest/PgPartitioningVersionTest.UniqueIndexRowsPersistenceAfterManualSplit/1
pgwrapper_pg_txn-test PgTxnTest.ShowEffectiveYBIsolationLevel
pgwrapper_pg_type-test PgTypeTest.YbTypeDetailsMatch
pgwrapper_pg_wait_on_conflict-test */PgWaitQueueRF1Test.TestDeadlockAcrossMultipleTablets/*
pgwrapper_pg_wait_on_conflict-test */PgWaitQueueRF1Test.TestDetectorPreservesBlockerSubtxnInfo/*
pgwrapper_pg_wait_on_conflict-test */PgWaitQueueRF1Test.TestResumingWaitersDoesntBlockTabletShutdown/*
Expand Down
12 changes: 7 additions & 5 deletions src/postgres/src/backend/access/common/heaptuple.c
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ heap_copytuple(HeapTuple tuple)
newTuple = (HeapTuple) palloc(HEAPTUPLESIZE + tuple->t_len);
newTuple->t_len = tuple->t_len;
newTuple->t_self = tuple->t_self;
HEAPTUPLE_COPY_YBITEM(tuple, newTuple);
HEAPTUPLE_COPY_YBCTID(tuple, newTuple);
newTuple->t_tableOid = tuple->t_tableOid;
newTuple->t_data = (HeapTupleHeader) ((char *) newTuple + HEAPTUPLESIZE);
memcpy((char *) newTuple->t_data, (char *) tuple->t_data, tuple->t_len);
Expand All @@ -724,7 +724,7 @@ heap_copytuple_with_tuple(HeapTuple src, HeapTuple dest)

dest->t_len = src->t_len;
dest->t_self = src->t_self;
HEAPTUPLE_COPY_YBITEM(src, dest);
HEAPTUPLE_COPY_YBCTID(src, dest);
dest->t_tableOid = src->t_tableOid;
dest->t_data = (HeapTupleHeader) palloc(src->t_len);
memcpy((char *) dest->t_data, (char *) src->t_data, src->t_len);
Expand All @@ -750,7 +750,7 @@ yb_heap_copytuple_with_tuple(HeapTuple src, HeapTuple dest)

dest->t_len = src->t_len;
dest->t_self = src->t_self;
HEAPTUPLE_COPY_YBITEM(src, dest);
HEAPTUPLE_COPY_YBCTID(src, dest);
dest->t_tableOid = src->t_tableOid;
/* assumes that dest->t_data is pre-allocated with enough memory. */
memcpy((char *) dest->t_data, (char *) src->t_data, src->t_len);
Expand Down Expand Up @@ -1115,6 +1115,7 @@ heap_form_tuple(TupleDesc tupleDescriptor,
tuple->t_len = len;
ItemPointerSetInvalid(&(tuple->t_self));
tuple->t_tableOid = InvalidOid;
HEAPTUPLE_YBCTID(tuple) = 0;

HeapTupleHeaderSetDatumLength(td, len);
HeapTupleHeaderSetTypeId(td, tupleDescriptor->tdtypeid);
Expand Down Expand Up @@ -1198,7 +1199,7 @@ heap_modify_tuple(HeapTuple tuple,
*/
newTuple->t_data->t_ctid = tuple->t_data->t_ctid;
newTuple->t_self = tuple->t_self;
HEAPTUPLE_COPY_YBITEM(tuple, newTuple);
HEAPTUPLE_COPY_YBCTID(tuple, newTuple);
newTuple->t_tableOid = tuple->t_tableOid;

return newTuple;
Expand Down Expand Up @@ -1262,7 +1263,7 @@ heap_modify_tuple_by_cols(HeapTuple tuple,
*/
newTuple->t_data->t_ctid = tuple->t_data->t_ctid;
newTuple->t_self = tuple->t_self;
HEAPTUPLE_COPY_YBITEM(tuple, newTuple);
HEAPTUPLE_COPY_YBCTID(tuple, newTuple);
newTuple->t_tableOid = tuple->t_tableOid;

return newTuple;
Expand Down Expand Up @@ -1504,6 +1505,7 @@ heap_tuple_from_minimal_tuple(MinimalTuple mtup)
result->t_len = len;
ItemPointerSetInvalid(&(result->t_self));
result->t_tableOid = InvalidOid;
HEAPTUPLE_YBCTID(result) = 0;
result->t_data = (HeapTupleHeader) ((char *) result + HEAPTUPLESIZE);
memcpy((char *) result->t_data + MINIMAL_TUPLE_OFFSET, mtup, mtup->t_len);
memset(result->t_data, 0, offsetof(HeapTupleHeaderData, t_infomask2));
Expand Down
1 change: 1 addition & 0 deletions src/postgres/src/backend/access/common/toast_internals.c
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ toast_save_datum(Relation rel, Datum value,
if (toastidxs[i]->rd_index->indisready)
index_insert(toastidxs[i], t_values, t_isnull,
&(toasttup->t_self),
toasttup->t_ybctid,
toastrel,
toastidxs[i]->rd_index->indisunique ?
UNIQUE_CHECK_YES : UNIQUE_CHECK_NO,
Expand Down
2 changes: 0 additions & 2 deletions src/postgres/src/backend/access/gin/ginpostinglist.c
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ itemptr_to_uint64(const ItemPointer iptr)
{
uint64 val;

Assert(YbItemPointerYbctid(iptr) == 0);
Assert(ItemPointerIsValid(iptr));
Assert(GinItemPointerGetOffsetNumber(iptr) < (1 << MaxHeapTuplesPerPageBits));

Expand All @@ -102,7 +101,6 @@ itemptr_to_uint64(const ItemPointer iptr)
static inline void
uint64_to_itemptr(uint64 val, ItemPointer iptr)
{
YbItemPointerSetInvalid(iptr);
GinItemPointerSetOffsetNumber(iptr, val & ((1 << MaxHeapTuplesPerPageBits) - 1));
val = val >> MaxHeapTuplesPerPageBits;
GinItemPointerSetBlockNumber(iptr, val);
Expand Down
1 change: 1 addition & 0 deletions src/postgres/src/backend/access/heap/heapam.c
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
scan->rs_inited = false;
scan->rs_ctup.t_data = NULL;
ItemPointerSetInvalid(&scan->rs_ctup.t_self);
HEAPTUPLE_YBCTID(&scan->rs_ctup) = (Datum) 0;
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;

Expand Down
9 changes: 8 additions & 1 deletion src/postgres/src/backend/access/heap/heapam_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -1172,7 +1172,8 @@ heapam_index_build_range_scan(Relation heapRelation,
void *callback_state,
TableScanDesc scan,
YbBackfillInfo *bfinfo,
YbPgExecOutParam *bfresult)
YbPgExecOutParam *bfresult,
YbIndexBuildCallback ybcallback)
{
HeapScanDesc hscan;
bool is_system_catalog;
Expand Down Expand Up @@ -1732,6 +1733,11 @@ heapam_index_build_range_scan(Relation heapRelation,
callback(indexRelation, &tid, values, isnull, tupleIsAlive,
callback_state);
}
else if (IsYBRelation(indexRelation))
{
ybcallback(indexRelation, heapTuple->t_ybctid, values, isnull,
tupleIsAlive, callback_state);
}
else
{
/* Call the AM's callback routine to process the tuple */
Expand Down Expand Up @@ -2010,6 +2016,7 @@ heapam_index_validate_scan(Relation heapRelation,
values,
isnull,
&rootTuple,
heapTuple->t_ybctid,
heapRelation,
indexInfo->ii_Unique ?
UNIQUE_CHECK_YES : UNIQUE_CHECK_NO,
Expand Down
4 changes: 2 additions & 2 deletions src/postgres/src/backend/access/heap/heaptoast.c
Original file line number Diff line number Diff line change
Expand Up @@ -491,8 +491,8 @@ yb_toast_compress_tuple(HeapTuple tup, TupleDesc tupleDesc)
new_tuple->t_data->t_infomask2 &= ~HEAP2_XACT_MASK;
new_tuple->t_data->t_infomask2 |=
tup->t_data->t_infomask2 & HEAP2_XACT_MASK;
HEAPTUPLE_COPY_YBITEM(tup, new_tuple);

HEAPTUPLE_COPY_YBCTID(tup, new_tuple);

/*
* Free allocated temp values
Expand Down
6 changes: 2 additions & 4 deletions src/postgres/src/backend/access/index/indexam.c
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ index_insert(Relation indexRelation,
Datum *values,
bool *isnull,
ItemPointer heap_t_ctid,
Datum ybctid,
Relation heapRelation,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
Expand All @@ -217,7 +218,7 @@ index_insert(Relation indexRelation,
if (IsYugaByteEnabled() && IsYBRelation(indexRelation))
{
return indexRelation->rd_indam->yb_aminsert(indexRelation, values, isnull,
heap_t_ctid, heapRelation,
ybctid, heapRelation,
checkUnique, indexInfo,
yb_shared_insert);
}
Expand Down Expand Up @@ -619,7 +620,6 @@ index_getnext_tid(IndexScanDesc scan, ScanDirection direction)
*/
Assert(IsYBRelation(scan->indexRelation) ?
(!ItemPointerIsValid(&scan->xs_heaptid) &&
YbItemPointerYbctid(&scan->xs_heaptid) == 0 &&
(!TupIsNull(scan->yb_agg_slot) ||
scan->xs_hitup != NULL ||
scan->xs_itup != NULL)) :
Expand Down Expand Up @@ -724,7 +724,6 @@ index_getnext_slot(IndexScanDesc scan, ScanDirection direction, TupleTableSlot *
*/
Assert(IsYBRelation(scan->indexRelation) ?
(!ItemPointerIsValid(&scan->xs_heaptid) &&
YbItemPointerYbctid(&scan->xs_heaptid) == 0 &&
(!TupIsNull(scan->yb_agg_slot) ||
scan->xs_hitup != NULL)) :
ItemPointerIsValid(&scan->xs_heaptid));
Expand All @@ -740,7 +739,6 @@ index_getnext_slot(IndexScanDesc scan, ScanDirection direction, TupleTableSlot *
*/
Assert(IsYBRelation(scan->indexRelation) ?
(!ItemPointerIsValid(&scan->xs_heaptid) &&
YbItemPointerYbctid(&scan->xs_heaptid) == 0 &&
(!TupIsNull(scan->yb_agg_slot) ||
scan->xs_hitup != NULL)) :
ItemPointerIsValid(&scan->xs_heaptid));
Expand Down
17 changes: 9 additions & 8 deletions src/postgres/src/backend/access/yb_access/yb_lsm.c
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,16 @@ doBindsForIdxWrite(YBCPgStatement stmt,
}

static void
ybcinbuildCallback(Relation index, ItemPointer tid, Datum *values, bool *isnull,
bool tupleIsAlive, void *state)
ybcinbuildCallback(Relation index, Datum ybctid, Datum *values,
bool *isnull, bool tupleIsAlive, void *state)
{
YBCBuildState *buildstate = (YBCBuildState *)state;

if (!buildstate->isprimary)
YBCExecuteInsertIndex(index,
values,
isnull,
tid,
ybctid,
buildstate->backfill_write_time,
doBindsForIdxWrite,
NULL /* indexstate */);
Expand All @@ -167,8 +167,9 @@ ybcinbuild(Relation heap, Relation index, struct IndexInfo *indexInfo)
*/
if (!index->rd_index->indisprimary)
{
heap_tuples = table_index_build_scan(heap, index, indexInfo, true, false,
ybcinbuildCallback, &buildstate, NULL);
heap_tuples = yb_table_index_build_scan(heap, index, indexInfo, true,
ybcinbuildCallback, &buildstate,
NULL);
}
/*
* Return statistics
Expand Down Expand Up @@ -218,7 +219,7 @@ ybcinbuildempty(Relation index)
}

static bool
ybcininsert(Relation index, Datum *values, bool *isnull, ItemPointer tid, Relation heap,
ybcininsert(Relation index, Datum *values, bool *isnull, Datum ybctid, Relation heap,
IndexUniqueCheck checkUnique, struct IndexInfo *indexInfo, bool sharedInsert)
{
if (!index->rd_index->indisprimary)
Expand All @@ -239,7 +240,7 @@ ybcininsert(Relation index, Datum *values, bool *isnull, ItemPointer tid, Relati
index,
values,
isnull,
tid,
ybctid,
NULL /* backfill_write_time */,
doBindsForIdxWrite,
NULL /* indexstate */);
Expand All @@ -250,7 +251,7 @@ ybcininsert(Relation index, Datum *values, bool *isnull, ItemPointer tid, Relati
YBCExecuteInsertIndex(index,
values,
isnull,
tid,
ybctid,
NULL /* backfill_write_time */,
doBindsForIdxWrite,
NULL /* indexstate */);
Expand Down
8 changes: 4 additions & 4 deletions src/postgres/src/backend/access/yb_access/yb_scan.c
Original file line number Diff line number Diff line change
Expand Up @@ -3665,7 +3665,7 @@ void ybcIndexCostEstimate(struct PlannerInfo *root, IndexPath *path,
}

static bool
YbFetchRowData(YBCPgStatement ybc_stmt, Relation relation, ItemPointer tid,
YbFetchRowData(YBCPgStatement ybc_stmt, Relation relation, Datum ybctid,
Datum *values, bool *nulls, YBCPgSysColumns *syscols)
{
bool has_data = false;
Expand All @@ -3675,7 +3675,7 @@ YbFetchRowData(YBCPgStatement ybc_stmt, Relation relation, ItemPointer tid,
YBCPgExpr ybctid_expr = YBCNewConstant(ybc_stmt,
BYTEAOID,
InvalidOid,
YbItemPointerYbctid(tid),
ybctid,
false);
HandleYBStatus(YBCPgDmlBindColumn(ybc_stmt, YBTupleIdAttributeNumber, ybctid_expr));

Expand Down Expand Up @@ -3707,7 +3707,7 @@ YbFetchRowData(YBCPgStatement ybc_stmt, Relation relation, ItemPointer tid,
}

bool
YbFetchHeapTuple(Relation relation, ItemPointer tid, HeapTuple* tuple)
YbFetchHeapTuple(Relation relation, Datum ybctid, HeapTuple* tuple)
{
bool has_data = false;
YBCPgStatement ybc_stmt;
Expand All @@ -3723,7 +3723,7 @@ YbFetchHeapTuple(Relation relation, ItemPointer tid, HeapTuple* tuple)
NULL /* prepare_params */,
YBCIsRegionLocal(relation),
&ybc_stmt));
has_data = YbFetchRowData(ybc_stmt, relation, tid, values, nulls, &syscols);
has_data = YbFetchRowData(ybc_stmt, relation, ybctid, values, nulls, &syscols);

/* Write into the given tuple */
if (has_data)
Expand Down
Loading

0 comments on commit b263910

Please sign in to comment.