Обсуждение: Assign TupleTableSlot->tts_tableOid duplicated in tale AM.
Hi hackers,
Recently, we discover that the field of tts_tableOid of TupleTableSlot is assigned duplicated in table AM's interface which is not necessary. For example, in table_scan_getnextslot,
```
static inline bool
table_scan_getnextslot(TableScanDesc sscan, ScanDirection direction, TupleTableSlot *slot)
{
slot->tts_tableOid = RelationGetRelid(sscan->rs_rd);
/*
* We don't expect direct calls to table_scan_getnextslot with valid
* CheckXidAlive for catalog or regular tables. See detailed comments in
* xact.c where these variables are declared.
*/
if (unlikely(TransactionIdIsValid(CheckXidAlive) && !bsysscan))
elog(ERROR, "unexpected table_scan_getnextslot call during logical decoding");
return sscan->rs_rd->rd_tableam->scan_getnextslot(sscan, direction, slot);
}
table_scan_getnextslot(TableScanDesc sscan, ScanDirection direction, TupleTableSlot *slot)
{
slot->tts_tableOid = RelationGetRelid(sscan->rs_rd);
/*
* We don't expect direct calls to table_scan_getnextslot with valid
* CheckXidAlive for catalog or regular tables. See detailed comments in
* xact.c where these variables are declared.
*/
if (unlikely(TransactionIdIsValid(CheckXidAlive) && !bsysscan))
elog(ERROR, "unexpected table_scan_getnextslot call during logical decoding");
return sscan->rs_rd->rd_tableam->scan_getnextslot(sscan, direction, slot);
}
```
we can see that it assigns tts_tableOid, while calling sscan->rs_rd->rd_tableam->scan_getnextslot which implemented by heap_getnextslot also assigns tts_tableOid in the call of ExecStoreBufferHeapTuple.
```
TupleTableSlot *
ExecStoreBufferHeapTuple(HeapTuple tuple,
TupleTableSlot *slot,
Buffer buffer)
{
/*
* sanity checks
*/
Assert(tuple != NULL);
Assert(slot != NULL);
Assert(slot->tts_tupleDescriptor != NULL);
Assert(BufferIsValid(buffer));
if (unlikely(!TTS_IS_BUFFERTUPLE(slot)))
elog(ERROR, "trying to store an on-disk heap tuple into wrong type of slot");
tts_buffer_heap_store_tuple(slot, tuple, buffer, false);
slot->tts_tableOid = tuple->t_tableOid;
return slot;
}
ExecStoreBufferHeapTuple(HeapTuple tuple,
TupleTableSlot *slot,
Buffer buffer)
{
/*
* sanity checks
*/
Assert(tuple != NULL);
Assert(slot != NULL);
Assert(slot->tts_tupleDescriptor != NULL);
Assert(BufferIsValid(buffer));
if (unlikely(!TTS_IS_BUFFERTUPLE(slot)))
elog(ERROR, "trying to store an on-disk heap tuple into wrong type of slot");
tts_buffer_heap_store_tuple(slot, tuple, buffer, false);
slot->tts_tableOid = tuple->t_tableOid;
return slot;
}
```
We can get the two assigned values are same by reading codes. Maybe we should remove one?
What's more, we find that maybe we assign slot->tts_tableOid in outer interface like table_scan_getnextslot may be better and more friendly when we import other pluggable storage formats. It can avoid duplicated assignments in every implementation of table AM's interfaces.
Regards,
Wenchao
On Tue, Sep 20, 2022 at 11:51 PM Wenchao Zhang <zwcpostgres@gmail.com> wrote: > We can get the two assigned values are same by reading codes. Maybe we should remove one? > > What's more, we find that maybe we assign slot->tts_tableOid in outer interface like table_scan_getnextslot may be betterand more friendly when we import other pluggable storage formats. I suppose that you're right; it really should happen in exactly one place, based on some overarching theory about how tts_tableOid works with the table AM interface. I just don't know what that theory is. -- Peter Geoghegan
Firstly, thank you for your reply.
Yeah, I think maybe just assigning tts_tableOid of TTS only once
during scanning the same table may be better. That really needs
to be thinked over.
Regards,
Wenchao
Peter Geoghegan <pg@bowt.ie> 于2022年9月28日周三 10:47写道:
On Tue, Sep 20, 2022 at 11:51 PM Wenchao Zhang <zwcpostgres@gmail.com> wrote:
> We can get the two assigned values are same by reading codes. Maybe we should remove one?
>
> What's more, we find that maybe we assign slot->tts_tableOid in outer interface like table_scan_getnextslot may be better and more friendly when we import other pluggable storage formats.
I suppose that you're right; it really should happen in exactly one
place, based on some overarching theory about how tts_tableOid works
with the table AM interface. I just don't know what that theory is.
--
Peter Geoghegan