Hi, > Some comments on the v3 patches: > > 1/ > + * > + * Note: use pg_class_aclcheck_ext() to detect a concurrent drop. > > This comment seems redundant - the function call, the missing_ok > check, and the warning message already make it clear.
Yeah, the code can explain itself clearly, so removed.
> 2/
> + * Note that the relation might not be locked, so it can be dropped
> concurrently.
> + * This can happen when doing a whole database vacuum or analyze in
> + * get_all_vacuum_rels(). We issue a WARNING log message and return false in
> + * this case.
>
> Instead of saying "relation might not be locked", can we be more
> explicit about the exact cases? For example: For database-wide
> vacuums, this function is called without holding a relation lock, so
> the table can be dropped concurrently. In that case, issue a WARNING
> and return false.
Fixed.
> I couldn't think of a good way to reproduce this other than creating a
> large number of tables to make the pg_class scan costlier. For
> predictable testing I might agree to adding an injection point.
> However, I have some comments:
>
> 3/
> +# Make sure that current user is not the owner of current database.
> +step setrole1
> +{
> + SET ROLE regress_vacuum;
> +}
>
> Is this necessary?
Yes, we won't check MAINTAIN privilege on the table if current user is the
owner of current database.
> 4/
> +step analyze1
> +{
> + ANALYZE;
> +}
>
> I think just testing VACUUM or VACUUM ANALYZE in a single test keeps
> things smaller, since the underlying code is the same for both.
Yeah, the ANALYZE might also increase the test time, so removed.
> 5/
> +++ b/src/test/modules/injection_points/specs/vacuum_concurrent_drop.spec
>
> Why do you need isolation tests with an injection point? Why not a TAP
> test under src/test/modules/test_misc? A TAP test with just VACUUM
> (not ANALYZE) would keep the test simpler with fewer lines of code.
The src/test/perl/README says:
It's used to drive tests for backup and restore, replication, etc - anything
that can't
really be expressed using pg_regress or the isolation test framework.
I can write a TAP test if we decide to use it.
> 6/
> + INJECTION_POINT("vacuum-constructing-vacuumable-rels", NULL);
>
> Nit: How about a shorter name like vacuum-get-rels or vacuum-build-rels?
I think the current one is ok, so I didn't change this.
--
Regards,
ChangAo Chen
v4-0001-Handle-concurrent-drop-when-doing-whole-database-.patch
Description: Binary data
v4-0002-Add-test-case-for-vacuum-with-a-concurrent-drop.patch
Description: Binary data
