Horton

Thread

Commits

  1. Centralize horizon determination for temp tables, fixing bug due to skew.

  2. Improve test coverage of ginvacuum.c.

  3. Set cutoff xmin more aggressively when vacuuming a temporary table.

  1. More aggressive vacuuming of temporary tables

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-28T15:46:49Z

    It strikes me that when we are vacuuming a temporary table (which
necessarily will be one of our own session), we don't really need
to care what the global xmin horizon is.  If we're not inside a
user transaction block, then there are no tuples in the table that
could be in-doubt anymore.  Neither are there any snapshots in our
session that could see any dead tuples.  Nor do we give a fig what
other sessions might think of those tuples.  So we could just set
the xmin cutoff as aggressively as possible, which is to say
equal to the nextXid counter.  While vacuuming a temp table is
perhaps not something people do very often, I think when they do
do it they would like us to clean out all the dead tuples not just
some.

Hence I propose 0001 attached.  80% of it is just API additions to allow
passing down the isTopLevel flag so that we can do the right thing in
the CLUSTER case; the important change is in vacuum_set_xid_limits.
(For ease of review, I didn't reindent the existing code in
vacuum_set_xid_limits, but that would be something to do before commit.)

The reason I got interested in this is that yesterday while fooling
with bug #16595, I was annoyed about our poor code test coverage in
access/gin/.  The 0002 patch attached brings coverage for ginvacuum.c
up from 59% to 93%; but as things stand, a long delay has to be inserted
between the DELETE and VACUUM steps, else the VACUUM won't remove the
dead tuples because of concurrent transactions, and we get no coverage
improvement.  Since the table in question is a temp table, that seems
pretty silly.

Thoughts?  Am I missing something important here?

			regards, tom lane

  2. Re: More aggressive vacuuming of temporary tables

    Michael Paquier <michael@paquier.xyz> — 2020-08-29T01:43:18Z

    On Fri, Aug 28, 2020 at 11:46:49AM -0400, Tom Lane wrote:
> Hence I propose 0001 attached.  80% of it is just API additions to allow
> passing down the isTopLevel flag so that we can do the right thing in
> the CLUSTER case; the important change is in vacuum_set_xid_limits.
> (For ease of review, I didn't reindent the existing code in
> vacuum_set_xid_limits, but that would be something to do before commit.)

I got to wonder lately if we should not have a static state like what
we do for MyXactFlags to track down if a utility query is a top-level
one or not as most of the time we just want to check the flag as
passed down by standard_ProcessUtility().  I have faced this problem
as well lately when pushing down a PreventInTransactionBlock() for
some stuff with REINDEX for example.  Not sure how reliable this would
be though..  Passing isTopLevel down the road is a no-brainer, and
perhaps having a static value would create more problems than it
solves in practice.
--
Michael

  3. Re: More aggressive vacuuming of temporary tables

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-29T16:17:54Z

    Michael Paquier <michael@paquier.xyz> writes:
> I got to wonder lately if we should not have a static state like what
> we do for MyXactFlags to track down if a utility query is a top-level
> one or not as most of the time we just want to check the flag as
> passed down by standard_ProcessUtility().  I have faced this problem
> as well lately when pushing down a PreventInTransactionBlock() for
> some stuff with REINDEX for example.  Not sure how reliable this would
> be though..  Passing isTopLevel down the road is a no-brainer, and
> perhaps having a static value would create more problems than it
> solves in practice.

Hm.  I suppose you'd need a PG_TRY block to ensure that the setting
got restored correctly after an error, so maintaining it that way
would be rather expensive.  Also it just doesn't seem like
transaction-wide state, so having a static for it feels like the
wrong thing.

[thinks for awhile...]  Would it make any sense to mark Portals as being
top-level or not, and then code that needs this info could look to
"ActivePortal->isTopLevel"?  We are already paying the PG_TRY overhead
to maintain the ActivePortal variable safely.  I'm not sure though
whether the Portal is granular enough.  Do we set up a separate
Portal for subcommands?

In the big scheme of things, though, we don't need this info in so
many places that passing it down as a parameter is an undue burden.

			regards, tom lane

  4. Re: More aggressive vacuuming of temporary tables

    Andres Freund <andres@anarazel.de> — 2020-09-08T19:01:29Z

    Hi,

On 2020-08-28 11:46:49 -0400, Tom Lane wrote:
> It strikes me that when we are vacuuming a temporary table (which
> necessarily will be one of our own session), we don't really need
> to care what the global xmin horizon is.  If we're not inside a
> user transaction block, then there are no tuples in the table that
> could be in-doubt anymore.  Neither are there any snapshots in our
> session that could see any dead tuples.  Nor do we give a fig what
> other sessions might think of those tuples.  So we could just set
> the xmin cutoff as aggressively as possible, which is to say
> equal to the nextXid counter.  While vacuuming a temp table is
> perhaps not something people do very often, I think when they do
> do it they would like us to clean out all the dead tuples not just
> some.

That seems like a good idea.

I've been toying with a patch that introduces more smarts about when a
row is removable, by looking more closely whether a specific row
versions are visible (e.g. in the common case of one old snapshot and
lots of newer rows). But that's orders of magnitude more complicated. So
going for something as simple as this seems like a good idea.


> Hence I propose 0001 attached.  80% of it is just API additions to allow
> passing down the isTopLevel flag so that we can do the right thing in
> the CLUSTER case; the important change is in vacuum_set_xid_limits.
> (For ease of review, I didn't reindent the existing code in
> vacuum_set_xid_limits, but that would be something to do before commit.)

I did wonder for a moment if it could be worthwhile to just implement
this for VACUUM and leave CLUSTER alone, to avoid having to deal with
the toplevel stuff. But I think it's better to be consistent.

But now I do wonder why we need to know whether the command is top level
or not? Why isn't the correct thing to instead look at what the current
backend's xmin is? Seems like you could just replace
    *oldestXmin = XidFromFullTransactionId(ReadNextFullTransactionId());
with
    *oldestXmin = MyProc->xmin;
    Assert(TransactionIdIsValid(*oldestXmin));

and you'd not need to care about whether the CLUSTER is top-level or
not? Without, I think, loosing any aggressiveness in the top-level case?
Perhaps even worth moving this logic into
GetOldestNonRemovableTransactionId().

I know you already pushed this, but I vote for revising it this way if
you don't see an issue?

Greetings,

Andres Freund

  5. Re: More aggressive vacuuming of temporary tables

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-08T19:24:54Z

    Andres Freund <andres@anarazel.de> writes:
> But now I do wonder why we need to know whether the command is top level
> or not? Why isn't the correct thing to instead look at what the current
> backend's xmin is? Seems like you could just replace
>     *oldestXmin = XidFromFullTransactionId(ReadNextFullTransactionId());
> with
>     *oldestXmin = MyProc->xmin;
>     Assert(TransactionIdIsValid(*oldestXmin));

Ummm ... since VACUUM doesn't run inside a transaction, it won't be
advertising an xmin will it?

Maybe you could make something like this work, but I think it'd still
have to treat CLUSTER as a special case.  Not sure it's worth it.

			regards, tom lane

  6. Re: More aggressive vacuuming of temporary tables

    Andres Freund <andres@anarazel.de> — 2020-09-08T21:47:53Z

    Hi,

On 2020-09-08 15:24:54 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > But now I do wonder why we need to know whether the command is top level
> > or not? Why isn't the correct thing to instead look at what the current
> > backend's xmin is? Seems like you could just replace
> >     *oldestXmin = XidFromFullTransactionId(ReadNextFullTransactionId());
> > with
> >     *oldestXmin = MyProc->xmin;
> >     Assert(TransactionIdIsValid(*oldestXmin));
> 
> Ummm ... since VACUUM doesn't run inside a transaction, it won't be
> advertising an xmin will it?

We do run it in a transaction though:

static bool
vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
{
...
	/* Begin a transaction for vacuuming this relation */
	StartTransactionCommand();

	/*
	 * Need to acquire a snapshot to prevent pg_subtrans from being truncated,
	 * cutoff xids in local memory wrapping around, and to have updated xmin
	 * horizons.
	 */
	PushActiveSnapshot(GetTransactionSnapshot());


> Maybe you could make something like this work, but I think it'd still
> have to treat CLUSTER as a special case.  Not sure it's worth it.

Why would CLUSTER need to be special cased? We'd precisely retain the
rows we need to, I think? Given that we'd exactly use the snapshot that
rules that determines which rows need to be retained?

Greetings,

Andres Freund

  7. Re: More aggressive vacuuming of temporary tables

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-08T22:13:58Z

    Andres Freund <andres@anarazel.de> writes:
> On 2020-09-08 15:24:54 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> But now I do wonder why we need to know whether the command is top level
>>> or not? Why isn't the correct thing to instead look at what the current
>>> backend's xmin is? Seems like you could just replace
>>> *oldestXmin = XidFromFullTransactionId(ReadNextFullTransactionId());
>>> with
>>> *oldestXmin = MyProc->xmin;
>>> Assert(TransactionIdIsValid(*oldestXmin));

>> Ummm ... since VACUUM doesn't run inside a transaction, it won't be
>> advertising an xmin will it?

> We do run it in a transaction though:

Ah.  But still, this is not the semantics I want for VACUUM, because the
process xmin will involve other processes' behavior.  The point of the
change as far as I'm concerned is to ensure vacuuming of dead temp rows
independent of anything else in the system.

			regards, tom lane

  8. Re: More aggressive vacuuming of temporary tables

    Stephen Frost <sfrost@snowman.net> — 2020-09-09T14:14:04Z

    Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2020-08-28 11:46:49 -0400, Tom Lane wrote:
> > It strikes me that when we are vacuuming a temporary table (which
> > necessarily will be one of our own session), we don't really need
> > to care what the global xmin horizon is.  If we're not inside a
> > user transaction block, then there are no tuples in the table that
> > could be in-doubt anymore.  Neither are there any snapshots in our
> > session that could see any dead tuples.  Nor do we give a fig what
> > other sessions might think of those tuples.  So we could just set
> > the xmin cutoff as aggressively as possible, which is to say
> > equal to the nextXid counter.  While vacuuming a temp table is
> > perhaps not something people do very often, I think when they do
> > do it they would like us to clean out all the dead tuples not just
> > some.
> 
> That seems like a good idea.

Agreed.

> I've been toying with a patch that introduces more smarts about when a
> row is removable, by looking more closely whether a specific row
> versions are visible (e.g. in the common case of one old snapshot and
> lots of newer rows). But that's orders of magnitude more complicated. So
> going for something as simple as this seems like a good idea.

I've wondered about this for a long time- very cool that you've found
time to actually work on a patch.  A couple of different ideas were
discussed previously about how to do that kind of a check- mind talking
about what method you're using, or perhaps just sharing that patch? :)

The potential of such an improvement is huge.

Thanks!

Stephen

  9. Re: More aggressive vacuuming of temporary tables

    Andres Freund <andres@anarazel.de> — 2020-09-09T18:02:05Z

    Hi,

On 2020-09-09 10:14:04 -0400, Stephen Frost wrote:
> > I've been toying with a patch that introduces more smarts about when a
> > row is removable, by looking more closely whether a specific row
> > versions are visible (e.g. in the common case of one old snapshot and
> > lots of newer rows). But that's orders of magnitude more complicated. So
> > going for something as simple as this seems like a good idea.
> 
> I've wondered about this for a long time- very cool that you've found
> time to actually work on a patch.  A couple of different ideas were
> discussed previously about how to do that kind of a check- mind talking
> about what method you're using, or perhaps just sharing that patch? :)

It's very very early, and it doesn't really work. I basically tried to
just plug a bit more intelligence into the dead tuple detection (which
now can easily store more and incrementally built state with the recent
changes for snapshot scalability).  Detection that tuples newer than the
horizon are dead isn't that complicated - what's hard is not breaking
things due to ctid chains lacking intermediate versions.  To avoid that
I had to restrict it to inserted (not updated) tuples that were
subsequently deleted. And my heuristic only supported only one old
snapshot.

Building a bsearchable list of ranges of valid (xmin-xmax] ranges isn't
that hard. Some care needs to be taken to make the list non-overlapping,
but that's easy enough by just merging entries.

Obviously lookup in such a more complicated structure isn't free. Nor is
building it. So we'd need some heuristics about when to do so. It'd
probably be OK to occasionally look at the age of the oldest in-progress
statement, to infer the time for old snapshots based on that. And then
we could have a GUC that says when it's worth doing more complicated
lookups.

I don't have a handle on how to deal with the ctid chaining for
intermediate row versions.

Greetings,

Andres Freund

  10. Re: More aggressive vacuuming of temporary tables

    Andres Freund <andres@anarazel.de> — 2020-10-14T20:31:03Z

    Hi,

On 2020-09-08 18:13:58 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-09-08 15:24:54 -0400, Tom Lane wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> >>> But now I do wonder why we need to know whether the command is top level
> >>> or not? Why isn't the correct thing to instead look at what the current
> >>> backend's xmin is? Seems like you could just replace
> >>> *oldestXmin = XidFromFullTransactionId(ReadNextFullTransactionId());
> >>> with
> >>> *oldestXmin = MyProc->xmin;
> >>> Assert(TransactionIdIsValid(*oldestXmin));
> 
> >> Ummm ... since VACUUM doesn't run inside a transaction, it won't be
> >> advertising an xmin will it?
> 
> > We do run it in a transaction though:
> 
> Ah.  But still, this is not the semantics I want for VACUUM, because the
> process xmin will involve other processes' behavior.  The point of the
> change as far as I'm concerned is to ensure vacuuming of dead temp rows
> independent of anything else in the system.

I was thinking about this a bit more, and I think the answer might be to
use Min(latestCompletedXid, MyProc->xid). That would, as far as I can
tell, never miss something vacuumable in a temporary table, doesn't
require to know whether we're running as the top-level command.

The reason for preferring latestCompletedXid over nextXid is that the
former is protected by ProcArrayLock and already accessed in
GetSnapshotData(), so we can cheaply compute the horizons as part of
pruning.

I think that cannot miss something vacuumable in a temp table for VACUUM
because that would have to have been left over by an already completed
transaction (by us, before the VACUUM).

In addition this allows HOT pruning etc on temp tables to be just as
aggressive as VACUUM is.

I wrote a patch to do so for [1], but it seemed topically more relevant
here. Running tests in a loop, no failures after the first few
iterations.

Greetings,

Andres Freund

[1] https://postgr.es/m/20200921212003.wrizvknpkim2whzo%40alap3.anarazel.de

  11. Re: More aggressive vacuuming of temporary tables

    Peter Geoghegan <pg@bowt.ie> — 2020-10-16T19:32:53Z

    On Wed, Sep 9, 2020 at 11:02 AM Andres Freund <andres@anarazel.de> wrote:
> Obviously lookup in such a more complicated structure isn't free. Nor is
> building it. So we'd need some heuristics about when to do so. It'd
> probably be OK to occasionally look at the age of the oldest in-progress
> statement, to infer the time for old snapshots based on that. And then
> we could have a GUC that says when it's worth doing more complicated
> lookups.
>
> I don't have a handle on how to deal with the ctid chaining for
> intermediate row versions.

I wonder if it makes sense to add the feature to nbtree first. This
side-steps the practical problem of having to figure out ctid
chaining. You can prove the idea in a relatively low risk way, while
still creating significant value for users.

We can reason about versioning within indexes without having
authoritative information about it close at hand. The trick is to
delay everything until it looks like we'll have to split the page. I
can see very substantial improvements to index bloat from non-HOT
updates with the deduplication-deletion patch I've been working on:

https://postgr.es/m/CAH2-WznjQQwSPSxBiZ6wQyBiKqFmfdjOdeqp28otqw551T7jcQ@mail.gmail.com

Importantly, the patch tends to bound the number of distinct index
entries *per logical row* in the presence of non-HOT updates (at least
for indexes that are not logically modified by the update). ISTM that
this is what really matters. The patch doesn't just reduce index bloat
-- it greatly caps the number of heap pages any given primary key
point lookup query will do. So in practice we eagerly kill precisely
the garbage index tuples that would have caused us the most trouble
without the new mechanism in place.

Avoiding a page split is a big win, so we can probably justify
relatively expensive lookup calls to make this work. This seems
especially likely to be true because we can probably ratchet up to
that only when we see that it will win. If a unique index has 10
entries for one value (10 versions), which is inevitable once HOT
pruning fails, then how many of those 10 do we really need? The answer
is perhaps just 2 or 3 maybe 99%+ of the time. I strongly suspect that
preventing page splits is more valuable than opportunistic heap
pruning (unless the pruning itself prevents page splits, which it may
or may not). Logically unnecessary page splits have obvious lasting
consequences, are usually highly correlated, and affect performance in
ways that are non-linear and likely very harmful in the real world.

--
Peter Geoghegan

  12. Re: More aggressive vacuuming of temporary tables

    Andres Freund <andres@anarazel.de> — 2020-10-29T01:11:47Z

    Hi,

On 2020-10-14 13:31:03 -0700, Andres Freund wrote:
> I was thinking about this a bit more, and I think the answer might be to
> use Min(latestCompletedXid, MyProc->xid). That would, as far as I can
> tell, never miss something vacuumable in a temporary table, doesn't
> require to know whether we're running as the top-level command.
> 
> The reason for preferring latestCompletedXid over nextXid is that the
> former is protected by ProcArrayLock and already accessed in
> GetSnapshotData(), so we can cheaply compute the horizons as part of
> pruning.
> 
> I think that cannot miss something vacuumable in a temp table for VACUUM
> because that would have to have been left over by an already completed
> transaction (by us, before the VACUUM).
> 
> In addition this allows HOT pruning etc on temp tables to be just as
> aggressive as VACUUM is.
> 
> I wrote a patch to do so for [1], but it seemed topically more relevant
> here. Running tests in a loop, no failures after the first few
> iterations.
> 
> [1] https://postgr.es/m/20200921212003.wrizvknpkim2whzo%40alap3.anarazel.de

Pushed this change in logic. The only "real" change is that the horizon
for backends without an xid needs to be latestCompletedXid + 1, rather
than just latestCompletedXid. The horizon indicates the oldest
*non*-removable xid, and for temp tables latestCompletedXid can always
be vacuumed when no xid is assigned.

Greetings,

Andres Freund