Re: Arbitary file size limit in twophase.c - Mailing list 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
Index: src/backend/access/transam/twophase.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/transam/twophase.c,v
retrieving revision 1.42
diff -c -r1.42 twophase.c
*** src/backend/access/transam/twophase.c    12 May 2008 00:00:45 -0000    1.42
--- src/backend/access/transam/twophase.c    16 May 2008 09:56:56 -0000
***************
*** 56,61 ****
--- 56,62 ----
  #include "storage/procarray.h"
  #include "storage/smgr.h"
  #include "utils/builtins.h"
+ #include "utils/memutils.h"


  /*
***************
*** 866,871 ****
--- 867,881 ----
      hdr->total_len = records.total_len + sizeof(pg_crc32);

      /*
+      * If the file size exceeds MaxAllocSize, we won't be able to read it in
+      * ReadTwoPhaseFile. Check for that now, rather than fail at commit time.
+      */
+     if (hdr->total_len > MaxAllocSize)
+         ereport(ERROR,
+                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+                  errmsg("two-phase state file maximum length exceeed")));
+
+     /*
       * Create the 2PC state file.
       *
       * Note: because we use BasicOpenFile(), we are responsible for ensuring
***************
*** 1044,1051 ****
      }

      /*
!      * 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.
       */
      if (fstat(fd, &stat))
      {
--- 1054,1060 ----
      }

      /*
!      * Check file length.  We can determine a lower bound pretty easily.
       */
      if (fstat(fd, &stat))
      {
***************
*** 1059,1066 ****

      if (stat.st_size < (MAXALIGN(sizeof(TwoPhaseFileHeader)) +
                          MAXALIGN(sizeof(TwoPhaseRecordOnDisk)) +
!                         sizeof(pg_crc32)) ||
!         stat.st_size > 10000000)
      {
          close(fd);
          return NULL;
--- 1068,1074 ----

      if (stat.st_size < (MAXALIGN(sizeof(TwoPhaseFileHeader)) +
                          MAXALIGN(sizeof(TwoPhaseRecordOnDisk)) +
!                         sizeof(pg_crc32)))
      {
          close(fd);
          return NULL;