SEO

Hannu Krosing wrote:
> *** src/backend/access/transam/xact.c 28 Apr 2005 21:47:10 -0000 1.200
> --- src/backend/access/transam/xact.c 17 May 2005 22:06:34 -0000
> ***************
> *** 1411,1416 ****
> --- 1411,1424 ----
> AfterTriggerBeginXact();
>
> /*
> + * mark the transaction as not VACUUM (vacuum_rel will set isVacuum to true
> + * directly after calling BeginTransactionCommand() )
> + */
> + if (MyProc != NULL)
> + {
> + MyProc->inVacuum = false;
> + }

I'm a little worried about having this set to "true" after a VACUUM is
executed, and only reset to false when the next transaction is begun: it
shouldn't affect correctness right now, but it seems like asking for
trouble. Resetting the flag to "false" after processing a transaction
would probably be worth doing.

> *** src/backend/commands/vacuum.c 6 May 2005 17:24:53 -0000 1.308
> --- src/backend/commands/vacuum.c 17 May 2005 22:06:35 -0000
> ***************
> *** 420,425 ****
> --- 418,428 ----
> if (use_own_xacts)
> {
> StartTransactionCommand();
> + if (MyProc != NULL) /* is this needed here ? */
> + {
> + /* so other vacuums don't look at our xid/xmin in GetOldestXmin() */
> + MyProc->inVacuum = true;
> + }
> /* functions in indexes may want a snapshot set */
> ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
> }

Is it valid to apply this optimization to ANALYZE? Since ANALYZE updates
pg_statistic, ISTM it can affect tuple visibility.

> *** src/backend/storage/ipc/sinval.c 31 Dec 2004 22:00:56 -0000 1.75
> --- src/backend/storage/ipc/sinval.c 17 May 2005 22:06:36 -0000
> ***************
> *** 845,854 ****
> * them as running anyway. We also assume that such xacts
> * can't compute an xmin older than ours, so they needn't be
> * considered in computing globalxmin.
> */
> if (proc == MyProc ||
> !TransactionIdIsNormal(xid) ||
> ! TransactionIdFollowsOrEquals(xid, xmax))
> continue;
>
> if (TransactionIdPrecedes(xid, xmin))
> --- 845,858 ----
> * them as running anyway. We also assume that such xacts
> * can't compute an xmin older than ours, so they needn't be
> * considered in computing globalxmin.
> + *
> + * there is also no need to consider transaxtions runnibg the
> + * vacuum command as it will not affect tuple visibility
> */

Typos.

-Neil

---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings

Neil Conway <neilc@samurai.com> writes:
> I'm a little worried about having this set to "true" after a VACUUM is
> executed, and only reset to false when the next transaction is begun: it
> shouldn't affect correctness right now, but it seems like asking for
> trouble. Resetting the flag to "false" after processing a transaction
> would probably be worth doing.

These days I'd be inclined to use a PG_TRY construct to guarantee the
flag is cleared, rather than loading another cleanup operation onto
unrelated code.

The MyProc != NULL tests are a waste of code space. You can't even
acquire an LWLock without MyProc being set, let alone access tables.

The real issue here though is whether anyone can blow a hole in the
xmin assumptions: is there any situation where ignoring a vacuum
transaction breaks things? I haven't had time to think about it
in any detail, but it definitely needs to be thought about.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org

On E, 2005-05-23 at 10:16 -0400, Tom Lane wrote:
> Neil Conway <neilc@samurai.com> writes:
> > I'm a little worried about having this set to "true" after a VACUUM is
> > executed, and only reset to false when the next transaction is begun: it
> > shouldn't affect correctness right now, but it seems like asking for
> > trouble. Resetting the flag to "false" after processing a transaction
> > would probably be worth doing.
>
> These days I'd be inclined to use a PG_TRY construct to guarantee the
> flag is cleared, rather than loading another cleanup operation onto
> unrelated code.

Ok, will check out PG_TRY. I hoped that there is some way not to set
inVacuum to false at each transaction start and still be sure that it
will be reverted after vacuum_rel.

So I'll set it once at the start of connection and then maintain it in
vacuum_rel() using PG_TRY.

> The MyProc != NULL tests are a waste of code space. You can't even
> acquire an LWLock without MyProc being set, let alone access tables.

Thanks, I'll get rid of them.

> The real issue here though is whether anyone can blow a hole in the
> xmin assumptions: is there any situation where ignoring a vacuum
> transaction breaks things? I haven't had time to think about it
> in any detail, but it definitely needs to be thought about.

There may be need to exclude vacuum/analyse on system relations from
being ignored by vacuum_rel() as I suspect that the info they both write
to pg_class, pg_attribute, and possibly other tables may be vulnerable
to crashes at right moment.

Also it may be prudent to not exclude other vacuums, when the vacuum_rel
() itself is run on a system relation.

I'm not sure which way it is, as my head gets all thick each time I try
to figure it out ;p.

I can't think of any other cases where it could matter, as at least the
work done inside vacuum_rel() itself seema non-rollbackable.

--
Hannu Krosing <hannu@skype.net>


---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Hannu Krosing <hannu@skype.net> writes:
> I can't think of any other cases where it could matter, as at least the
> work done inside vacuum_rel() itself seema non-rollbackable.

VACUUM FULL's tuple-moving is definitely roll-back-able, so it might be
prudent to only do this for lazy VACUUM. But on the other hand, VACUUM
FULL holds an exclusive lock on the table so no one else is going to see
its effects concurrently anyway.

As I said, it needs more thought than I've been able to spare for it yet
....

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings

On E, 2005-05-23 at 11:42 -0400, Tom Lane wrote:
> Hannu Krosing <hannu@skype.net> writes:
> > I can't think of any other cases where it could matter, as at least the
> > work done inside vacuum_rel() itself seema non-rollbackable.
>
> VACUUM FULL's tuple-moving is definitely roll-back-able, so it might be
> prudent to only do this for lazy VACUUM. But on the other hand, VACUUM
> FULL holds an exclusive lock on the table so no one else is going to see
> its effects concurrently anyway.

I'm not interested in VACUUM FULL at all. This is improvement mainly for
heavy update OLAP databases, where I would not even think of running
VACUUM FULL.

I'll cheks if there's an easy way to exclude VACUUM FULL.

> As I said, it needs more thought than I've been able to spare for it yet
> ...

Ok, thanks for comments this far .

--
Hannu Krosing <hannu@skype.net>


---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

On E, 2005-05-23 at 11:42 -0400, Tom Lane wrote:
> Hannu Krosing <hannu@skype.net> writes:
> > I can't think of any other cases where it could matter, as at least the
> > work done inside vacuum_rel() itself seema non-rollbackable.
>
> VACUUM FULL's tuple-moving is definitely roll-back-able, so it might be
> prudent to only do this for lazy VACUUM. But on the other hand, VACUUM
> FULL holds an exclusive lock on the table so no one else is going to see
> its effects concurrently anyway.

Ok, this is a new version of the vacuum patch with the following changes
following some suggestions in this thread.

* changed the patch to affect only lazy vacuum
* moved inVacuum handling to use PG_TRY
* moved vac_update_relstats() out of lazy_vacuum_rel into a separate
transaction. The code to do this may not be the prettiest, maybe it
should use a separate struct.

--
Hannu Krosing <hannu@skype.net>


---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Hannu Krosing <hannu@skype.net> writes:
> Ok, this is a new version of the vacuum patch with the following changes
> following some suggestions in this thread.

The more I look at this, the uglier it looks ... and I still haven't
seen any convincing demonstration that it *works*, ie doesn't have
bad side-effects on the transaction-is-in-progress logic. I'm
particularly concerned about what happens to the RecentXmin horizon
for pg_subtrans and pg_multixact operations.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

On P, 2005-07-03 at 12:19 -0400, Tom Lane wrote:
> Hannu Krosing <hannu@skype.net> writes:
> > Ok, this is a new version of the vacuum patch with the following changes
> > following some suggestions in this thread.
>
> The more I look at this, the uglier it looks ... and I still haven't
> seen any convincing demonstration that it *works*, ie doesn't have
> bad side-effects on the transaction-is-in-progress logic.

The function GetOldestXmin is used *only* when determining oldest xmin
for transactions.

> I'm particularly concerned about what happens to the RecentXmin horizon
> for pg_subtrans and pg_multixact operations.

How are they affected by this change ? They should still see the vacuum
as oldest transaction, unless they


Oh, now I see. I'm pretty sure that at the time of original patch, the
*only* uses of GetOldestXmin was from VACUUM and catalog/index.c and
both for the same purpose, but now I see also a call from
access/transam/xlog.c.

Perhaps I should separate the function used by vacuum into another
function, say GetOldestDataChangingXmin(), to keep the possible impact
as localised as possible.

Do you have any specific concerns related to this patch after that ?

Or should I just back off for now and maybe start a separate project for
ironing out patches related to running postgresql in real-world 24/7
OLTP environment (similar to what Bizgres does for OLAP ) ?

--
Hannu Krosing <hannu@skype.net>



---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Alvaro has just applied a modified version of this patch.

---------------------------------------------------------------------------

Hannu Krosing wrote:
> On E, 2005-05-23 at 11:42 -0400, Tom Lane wrote:
> > Hannu Krosing <hannu@skype.net> writes:
> > > I can't think of any other cases where it could matter, as at least the
> > > work done inside vacuum_rel() itself seema non-rollbackable.
> >
> > VACUUM FULL's tuple-moving is definitely roll-back-able, so it might be
> > prudent to only do this for lazy VACUUM. But on the other hand, VACUUM
> > FULL holds an exclusive lock on the table so no one else is going to see
> > its effects concurrently anyway.
>
> Ok, this is a new version of the vacuum patch with the following changes
> following some suggestions in this thread.
>
> * changed the patch to affect only lazy vacuum
> * moved inVacuum handling to use PG_TRY
> * moved vac_update_relstats() out of lazy_vacuum_rel into a separate
> transaction. The code to do this may not be the prettiest, maybe it
> should use a separate struct.
>
> --
> Hannu Krosing <hannu@skype.net>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Bruce Momjian wrote:
>
> Alvaro has just applied a modified version of this patch.

Hannu, I'm curious:

> Hannu Krosing wrote:

> > Ok, this is a new version of the vacuum patch with the following changes
> > following some suggestions in this thread.
> >
> > * changed the patch to affect only lazy vacuum
> > * moved inVacuum handling to use PG_TRY
> > * moved vac_update_relstats() out of lazy_vacuum_rel into a separate
> > transaction. The code to do this may not be the prettiest, maybe it
> > should use a separate struct.

What was idea behind moving vac_update_relstats to a separate
transaction? I'm wondering if it's still needed, if it further enhances
the system somehow, or your patch did something differently than what
was applied.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend