The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
Hi Paul,
Thanks for this patch!
Lilian and I have reviewed your patches.
All patches apply and all tests are passing.
Since you are adding tests, we expected to see some improvement on the test
coverage. Yet no significant improvement was observed. We have tested vanila
postgres at commit 931766aaec58b2ce09c82203456877e0b05e1271, run coverage.
Then we have applied first patch v1-0001, tested and run coverage.
Lastly v2-0001 was applied on top of v1-0001, tested and run coverage.
All three coverage reports can be found here [1].
We expected some increase in coverage especially in this file
`src/backend/utils/adt/ri_triggers.c` perhaps we are looking at the wrong
file?
In addition to that, related to the comment
'Violates referential integrity unless we use an up-to-date crosscheck snapshot:'
Could you please clarify:
Why is this necessary to specify usage of an up-to-date crosscheck snapshot?
When would the crosscheck snapshot be not up to date?
Should we add tests for up-to-date crosscheck snapshots?
Also in v1-0002 you only test 'REPEATABLE READ' isolation level, what about the
other two? ('READ COMMITTED', 'SERIALIZABLE')
Another question we wanted to clarify, what would be the right bash script
to test this specific feature only instead of running the whole test suit.
These are the steps we came up with so far: (we use Macos)
./configure --prefix=$PGINSTALL/paul --enable-cassert --enable-debug \
--enable-coverage --enable-tap-tests --without-icu --without-zstd \
--without-zlib CFLAGS="-ggdb -O0 -fno-omit-frame-pointer" CPPFLAGS="-g -O0"
make -j12 -s
make -j12 install
cd src/test/isolation
make check TESTNAME="fk-snapshot fk-snapshot-2 fk-snapshot-3"
make coverage-html
Kindest regards.
[1] https://drive.google.com/drive/folders/17Yc8C9p0foZSnT-PtIXgfiFlAY80V4Mc
The new status of this patch is: Waiting on Author
