Thread: [PATCH] Avoid open and lock the table Extendend Statistics (src/backend/commands/statscmds.c)

Hi,

On 2022-02-13 18:21:38 -0300, Ranier Vilela wrote:
> Why open and lock the table Extended Statistics if it is not the owner.
> Check and return to avoid this.

I was about to say that this opens up time-to-check-time-to-use type
issues. But it's the wrong thing to lock to prevent those.

Having looked briefly at it, I don't understand what the locking scheme is?
Shouldn't there be at least some locking against concurrent ALTERs and between
ALTER and ANALYZE etc?

Greetings,

Andres Freund

On 2/13/22 22:43, Andres Freund wrote:
> Hi,
> 
> On 2022-02-13 18:21:38 -0300, Ranier Vilela wrote:
>> Why open and lock the table Extended Statistics if it is not the owner.
>> Check and return to avoid this.
> 
> I was about to say that this opens up time-to-check-time-to-use type
> issues. But it's the wrong thing to lock to prevent those.
> 

I doubt optimizing this is worth any effort - ALTER STATISTICS is rare,
not doing it with owner rights is even rarer.


> Having looked briefly at it, I don't understand what the locking scheme is?
> Shouldn't there be at least some locking against concurrent ALTERs and between
> ALTER and ANALYZE etc?
> 

I don't recall the details of the discussion (if at all), but if you try
to do concurrent ALTER STATISTICS, that'll end up with:

  ERROR: tuple concurrently updated

reported by CatalogTupleUpdate. AFAIK that's what we do for other object
types that don't have a relation that we might lock (e.g. try to co
CREATE OR REPLACE FUNCTION).

ANALYZE reads the statistics from the catalog, so it should see the last
committed stattarget value, I think.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> On 2/13/22 22:43, Andres Freund wrote:
>> Having looked briefly at it, I don't understand what the locking scheme is?

> I don't recall the details of the discussion (if at all), but if you try
> to do concurrent ALTER STATISTICS, that'll end up with:
>   ERROR: tuple concurrently updated
> reported by CatalogTupleUpdate. AFAIK that's what we do for other object
> types that don't have a relation that we might lock (e.g. try to co
> CREATE OR REPLACE FUNCTION).

Yeah, we generally haven't bothered with a separate locking scheme
for objects other than relations.  I don't see any big problem with
that for objects that are defined by a single catalog entry, since
"tuple concurrently updated" failures serve fine (although maybe
that error message could be nicer).  Extended stats don't quite
fit that definition, but at least for updates that only need to
touch the pg_statistic_ext row, it's the same story.

When we get around to supporting multi-relation statistics,
there might need to be some protocol around when ANALYZE can
update pg_statistic_ext_data rows.  But I don't think that
issue exists yet, since only one ANALYZE can run on a particular
relation at a time.

            regards, tom lane