On Wed, Sep 16, 2020 at 10:40 AM Ashutosh Sharma <ashu.coe...@gmail.com> wrote: > > On Wed, Sep 16, 2020 at 9:14 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > Ashutosh Sharma <ashu.coe...@gmail.com> writes: > > > On Wed, Sep 16, 2020 at 1:25 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > >> * Should any of the other tables in the test be converted to temp? > > > > > Are you trying to say that we can achieve the things being done in > > > test-case 1 and 2 by having a single temp table and we should aim for > > > it because it will make the test-case more efficient and easy to > > > maintain? > > > > Well, I'm just looking at the comment that says the reason for the > > begin/rollback structure is to keep autovacuum's hands off the table. > > In most if not all of the other places where we need that, the preferred > > method is to make the table temp or mark it with (autovacuum = off). > > While this way isn't wrong exactly, nor inefficient, it does seem > > a little restrictive. For instance, you can't easily test cases that > > involve intentional errors. > > > > Another point is that we have a few optimizations that apply to tables > > created in the current transaction. I'm not sure whether any of them > > would fire in this test case, but if they do (now or in the future) > > that might mean you aren't testing the usual scenario for use of > > pg_surgery, which is surely not going to be a new-in-transaction > > table. (That might be an argument for preferring autovacuum = off > > over a temp table, too.) > > > > I agree with you on both the above points. I'll try to make the > necessary changes to address all your comments. Also, I'd prefer > creating a normal heap table with autovacuum = off over the temp table > for the reasons you mentioned in the second point. >
Attached is the patch with the changes suggested here. I've tried to use a normal heap table with (autovacuum = off) wherever possible. Please have a look and let me know for any other issues. Thanks, -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com
diff --git a/contrib/pg_surgery/expected/heap_surgery.out b/contrib/pg_surgery/expected/heap_surgery.out index 9451c57..033e3aa 100644 --- a/contrib/pg_surgery/expected/heap_surgery.out +++ b/contrib/pg_surgery/expected/heap_surgery.out @@ -1,8 +1,8 @@ create extension pg_surgery; -- create a normal heap table and insert some rows. --- note that we don't commit the transaction, so autovacuum can't interfere. -begin; -create table htab(a int); +-- note that we don't want autovacuum to interfere and therefore we disable it +-- on the test table. +create table htab(a int) with (autovacuum_enabled = off); insert into htab values (100), (200), (300), (400), (500); -- test empty TID array select heap_force_freeze('htab'::regclass, ARRAY[]::tid[]); @@ -85,9 +85,10 @@ NOTICE: skipping tid (0, 6) for relation "htab" because the item number is out (1 row) -rollback; -- set up a new table with a redirected line pointer -create table htab2(a int) with (autovacuum_enabled = off); +-- note that unlike other test cases we need a temp table here to ensure that we +-- get a stable vacuum results. +create temp table htab2(a int); insert into htab2 values (100); update htab2 set a = 200; vacuum htab2; @@ -175,6 +176,6 @@ ERROR: "vw" is not a table, materialized view, or TOAST table select heap_force_freeze('vw'::regclass, ARRAY['(0, 1)']::tid[]); ERROR: "vw" is not a table, materialized view, or TOAST table -- cleanup. -drop table htab2; +drop table htab; drop view vw; drop extension pg_surgery; diff --git a/contrib/pg_surgery/sql/heap_surgery.sql b/contrib/pg_surgery/sql/heap_surgery.sql index 8a27214..234e667 100644 --- a/contrib/pg_surgery/sql/heap_surgery.sql +++ b/contrib/pg_surgery/sql/heap_surgery.sql @@ -1,9 +1,9 @@ create extension pg_surgery; -- create a normal heap table and insert some rows. --- note that we don't commit the transaction, so autovacuum can't interfere. -begin; -create table htab(a int); +-- note that we don't want autovacuum to interfere and therefore we disable it +-- on the test table. +create table htab(a int) with (autovacuum_enabled = off); insert into htab values (100), (200), (300), (400), (500); -- test empty TID array @@ -38,10 +38,10 @@ select ctid, xmax from htab where xmin = 2; -- out-of-range TIDs should be skipped select heap_force_freeze('htab'::regclass, ARRAY['(0, 0)', '(0, 6)']::tid[]); -rollback; - -- set up a new table with a redirected line pointer -create table htab2(a int) with (autovacuum_enabled = off); +-- note that unlike other test cases we need a temp table here to ensure that we +-- get a stable vacuum results. +create temp table htab2(a int); insert into htab2 values (100); update htab2 set a = 200; vacuum htab2; @@ -86,6 +86,6 @@ select heap_force_kill('vw'::regclass, ARRAY['(0, 1)']::tid[]); select heap_force_freeze('vw'::regclass, ARRAY['(0, 1)']::tid[]); -- cleanup. -drop table htab2; +drop table htab; drop view vw; drop extension pg_surgery;