Regarding t_cid in Neon heap WAL records

[hlinnaka]

Reported by Muhammad Malik on pgsql-hackers:

https://www.postgresql.org/message-id/EA2P220MB0947DC092E176F5AEA297520A6AA2%40EA2P220MB0947.NAMP220.PROD.OUTLOOK.COM

Neon added a t_cid field to heap WAL records https://github.com/yibit/neon-postgresql/blob/main/docs/core_changes.md#add-t_cid-to-heap-wal-records.

However, when replaying the delete log record, it is discarding the combo flag and storing the raw cmax on the old tuple https://github.com/neondatabase/neon/blob/main/pgxn/neon_rmgr/neon_rmgr.c#L376. This will make the tuple header different from what is in the buffer cache if the deleted tuple was using a combocid. Similarly, there was no t_cid added for the old tuple in xl_neon_heap_update, and it is using the t_cid of the new tuple to set cmax on the old tuple during redo_neon_heap_update.

Why is this not a problem when a visibility check is performed on the tuple after reading from storage, since it won't get the correct cmin value on the old tuple?
Also, what is the need of adding the t_cid of the new tuple in xl_neon_heap_update when it is already present in the xl_neon_heap_header? Seems like it is sending the same t_cid twice with the update WAL record.
Thanks,
Muhammad

[hlinnaka]

However, when replaying the delete log record, it is discarding the combo flag and storing the raw cmax on the old tuple https://github.com/neondatabase/neon/blob/main/pgxn/neon_rmgr/neon_rmgr.c#L376. This will make the tuple header different from what is in the buffer cache if the deleted tuple was using a combocid.

Hmm, yes I think you're right, we're not setting the HEAP_COMBOCID flag correctly. Thanks for the report!

Similarly, there was no t_cid added for the old tuple in xl_neon_heap_update, and it is using the t_cid of the new tuple to set cmax on the old tuple during redo_neon_heap_update.

Why is this not a problem when a visibility check is performed on the tuple after reading from storage, since it won't get the correct cmin value on the old tuple?

It probably does cause problems; we apparently don't have enough test coverage for combocids.

Also, what is the need of adding the t_cid of the new tuple in xl_neon_heap_update when it is already present in the xl_neon_heap_header? Seems like it is sending the same t_cid twice with the update WAL record.

Hmm, yeah, if no combocids are involved, the old and the new tuple will have the same command id, as cmax on the old tuple and as cmin on the new tuple. But with a combocid, that's not so, so we need to store both in the WAL record. But we're missing setting the HEAP_COMBOCID flag there as well.

[knizhnik]

I wonder if it is enough just to store t_cid directory instead of using HeapTupleHeaderSetCmin/Cmax macros which affect HEAP_COMBOCID flag. This flag is already stored in t_infomask - no need to somehow change it in redo handler.

#8503

[knizhnik]

Looks like our current test (test_runner/sql_regress/sql/neon-cid.sql) is not testing CID handling at all.
At least I have commented assignment of t_cid in neon_rmgr.c and test_pg_regress still passed.

As far as I understand, to reproduce the problem we need to make some changes in transaction, then force throwing away this pages from cache and then access them once again.

I have added in my PR test_combocid.py - it really reproduces incorrect behaviour if we are not restoring CID.
But I failed to to reproduce the problem with COMBOCID.

[ololobus]

This week:

• Heikki: have a look on Preserve HEAP_COMBOCID when restoring t_cid from WAL #8503 and maybe suggest something

[ololobus]

This week:

• Heikki: approve Postgres part of Preserve HEAP_COMBOCID when restoring t_cid from WAL #8503