There's an apparently arbitary limit of 10,000,000 bytes in twophase.c
on the size of a two phase commit file. I can't see why this limit
exists.

I hit this limit by creating a prepared transaction which included
dropping a schema with about 25,000 objects in it and then trying to
commit it. The result is that ReadTwoPhaseFile() returns NULL and
FinishPreparedTransaction() says it detected a corrupted file.

Thoughts?

Thanks,

Gavin

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Gavin Sherry <swm@alcove.com.au> writes:
> There's an apparently arbitary limit of 10,000,000 bytes in twophase.c
> on the size of a two phase commit file. I can't see why this limit
> exists.

The comment seems perfectly clear about why the limit exists:

* Check file length. We can determine a lower bound pretty easily. We
* set an upper bound mainly to avoid palloc() failure on a corrupt file.

although certainly the specific value has been picked out of the air.

Perhaps it'd be better to use malloc() than palloc(), so that we'd not
lose control on out-of-memory, and then deem the file "too big" only
if we couldn't malloc the space.

Or we could try to fix things so that the file doesn't have to be all in
memory, but that seems pretty invasive.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, May 13, 2008 at 10:34:23AM -0400, Tom Lane wrote:
> Gavin Sherry <swm@alcove.com.au> writes:
> > There's an apparently arbitary limit of 10,000,000 bytes in twophase.c
> > on the size of a two phase commit file. I can't see why this limit
> > exists.
>
> The comment seems perfectly clear about why the limit exists:
>
> * Check file length. We can determine a lower bound pretty easily. We
> * set an upper bound mainly to avoid palloc() failure on a corrupt file.

Oops. Where was my brain?

> Perhaps it'd be better to use malloc() than palloc(), so that we'd not
> lose control on out-of-memory, and then deem the file "too big" only
> if we couldn't malloc the space.

That seems better.

Thanks,

Gavin

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:
> Gavin Sherry <swm@alcove.com.au> writes:
>> There's an apparently arbitary limit of 10,000,000 bytes in twophase.c
>> on the size of a two phase commit file. I can't see why this limit
>> exists.
>
> The comment seems perfectly clear about why the limit exists:
>
> * Check file length. We can determine a lower bound pretty easily. We
> * set an upper bound mainly to avoid palloc() failure on a corrupt file.
>
> although certainly the specific value has been picked out of the air.

I'm surprised the twophase file grows that big. If you exceed 10MB by
dropping 25000 objects, that's 400 bytes per object. A
TwoPhaseLockRecord is only 20-24 bytes + TwoPhaseRecordOnDisk header
which is 8 bytes, so what's taking so much space?

[tests...] It looks like it's the shared cache invalidation messages
that make up the rest of the bulk. They could probably be stored in a
more dense format, we currently store each invalidation message as a
separate twophase-record. I don't think I want to spend time on that
myself, but if someone cares enough to write a patch I'll be happy to
review it.

> Perhaps it'd be better to use malloc() than palloc(), so that we'd not
> lose control on out-of-memory, and then deem the file "too big" only
> if we couldn't malloc the space.

That doesn't seem much better to me than just letting the palloc fail.

If we're going to check for file length, we should definitely check the
file length when we write it, so that we fail at PREPARE time, and not
at COMMIT time.

I'll write up a patch along the lines of:
1. increase the limit to, say, 100 MB
2. Check the file length at PREPARE time.

> Or we could try to fix things so that the file doesn't have to be all in
> memory, but that seems pretty invasive.

Yep. And definitely not back-patchable.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> If we're going to check for file length, we should definitely check the
> file length when we write it, so that we fail at PREPARE time, and not
> at COMMIT time.

I think this is mere self-delusion, unfortunately. You can never be
certain at prepare time that a large alloc will succeed sometime later
in a different process.

Gavin's complaint is essentially that a randomly chosen hard limit is
bad, and I agree with that. Choosing a larger hard limit doesn't make
it less random.

It might be worth checking at prepare that the file size doesn't exceed
MaxAllocSize, but any smaller limit strikes me as (a) unnecessarily
restrictive and (b) not actually creating any useful guarantee.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:
> "Heikki Linnakangas" <heikki@enterprisedb.com> writes:
>> If we're going to check for file length, we should definitely check the
>> file length when we write it, so that we fail at PREPARE time, and not
>> at COMMIT time.
>
> I think this is mere self-delusion, unfortunately. You can never be
> certain at prepare time that a large alloc will succeed sometime later
> in a different process.
>
> Gavin's complaint is essentially that a randomly chosen hard limit is
> bad, and I agree with that. Choosing a larger hard limit doesn't make
> it less random.
>
> It might be worth checking at prepare that the file size doesn't exceed
> MaxAllocSize, but any smaller limit strikes me as (a) unnecessarily
> restrictive and (b) not actually creating any useful guarantee.

Hmm, I guess you're right.

Patch attached. I can't commit it myself right now, but will do so as
soon as I can, unless there's objections.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> Tom Lane wrote:
>> It might be worth checking at prepare that the file size doesn't exceed
>> MaxAllocSize, but any smaller limit strikes me as (a) unnecessarily
>> restrictive and (b) not actually creating any useful guarantee.

> Patch attached. I can't commit it myself right now, but will do so as
> soon as I can, unless there's objections.

Two bugs: "exceeed" -> "exceeded", please; and on the read side, you
should still have an upper-bound check, but it should be MaxAllocSize.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:
> "Heikki Linnakangas" <heikki@enterprisedb.com> writes:
>> Tom Lane wrote:
>>> It might be worth checking at prepare that the file size doesn't exceed
>>> MaxAllocSize, but any smaller limit strikes me as (a) unnecessarily
>>> restrictive and (b) not actually creating any useful guarantee.
>
>> Patch attached. I can't commit it myself right now, but will do so as
>> soon as I can, unless there's objections.
>
> Two bugs: "exceeed" -> "exceeded", please;

Thanks.

> and on the read side, you
> should still have an upper-bound check, but it should be MaxAllocSize.

That seems like a highly unlikely failure scenario, where a two-phase
file is corrupt file so that it becomes larger than 1GB. It's not like
the check costs anything either, though, so I'll just put it back like
you suggested.

Updated patch attached. I think it's ok now, but I'll air this as a
patch before committing since I got it wrong before...

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers