Re: Handle concurrent drop when doing whole database vacuum

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