Thread: minor fix of elevel in fd.c

minor fix of elevel in fd.c

From

"Qingqing Zhou"

Date:

12 June 2006, 05:57:26

AllocateFile() and AllocateDir() should return the control to the caller
since we might want to upgrade the elevel.

BTW: Seems this premature-elog-error problem (we talked about it in an old
thread and fix the dynahash code) also exists in some other places (e.g. the
assign_hook functions), but not sure if those deserve a fix.

Regards,
Qingqing

---

Index: src/backend/storage/file/fd.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/file/fd.c,v
retrieving revision 1.128
diff -c -r1.128 fd.c
*** src/backend/storage/file/fd.c       30 May 2006 13:04:59 -0000
1.128
--- src/backend/storage/file/fd.c       12 Jun 2006 05:43:00 -0000
***************
*** 1251,1257 ****
         */
        if (numAllocatedDescs >= MAX_ALLOCATED_DESCS ||
                numAllocatedDescs >= max_safe_fds - 1)
!               elog(ERROR, "too many private files demanded");

  TryAgain:
        if ((file = fopen(name, mode)) != NULL)
--- 1251,1261 ----
         */
        if (numAllocatedDescs >= MAX_ALLOCATED_DESCS ||
                numAllocatedDescs >= max_safe_fds - 1)
!       {
!               errno = 0;
!               elog(LOG, "too many private files demanded");
!               return NULL;
!       }

  TryAgain:
        if ((file = fopen(name, mode)) != NULL)
***************
*** 1366,1372 ****
         */
        if (numAllocatedDescs >= MAX_ALLOCATED_DESCS ||
                numAllocatedDescs >= max_safe_fds - 1)
!               elog(ERROR, "too many private dirs demanded");

  TryAgain:
        if ((dir = opendir(dirname)) != NULL)
--- 1370,1380 ----
         */
        if (numAllocatedDescs >= MAX_ALLOCATED_DESCS ||
                numAllocatedDescs >= max_safe_fds - 1)
!       {
!               errno = 0;
!               elog(LOG, "too many private dirs demanded");
!               return NULL;
!       }

  TryAgain:
        if ((dir = opendir(dirname)) != NULL)

Re: minor fix of elevel in fd.c

From

Tom Lane

Date:

12 June 2006, 13:47:38

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> AllocateFile() and AllocateDir() should return the control to the caller
> since we might want to upgrade the elevel.

That is not what we do for upgrading errors.  Use a critical section in
a caller that doesn't want elog(ERROR).

            regards, tom lane

Re: minor fix of elevel in fd.c

From

Qingqing Zhou

Date:

13 June 2006, 01:45:26

On Mon, 12 Jun 2006, Tom Lane wrote:

> "Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> > AllocateFile() and AllocateDir() should return the control to the caller
> > since we might want to upgrade the elevel.
>
> That is not what we do for upgrading errors.  Use a critical section in
> a caller that doesn't want elog(ERROR).
>

True, but current code just check the return value of AllocateABC() then
decide to upgrade elevel to FATAL. This is ok if we allow AllocateABC()
don't elog(ERROR) themselves.

Regards,
Qingqing

Re: minor fix of elevel in fd.c

From

Tom Lane

Date:

13 June 2006, 01:48:00

Qingqing Zhou <zhouqq@cs.toronto.edu> writes:
> On Mon, 12 Jun 2006, Tom Lane wrote:
>> That is not what we do for upgrading errors.  Use a critical section in
>> a caller that doesn't want elog(ERROR).

> True, but current code just check the return value of AllocateABC() then
> decide to upgrade elevel to FATAL.

Where?

            regards, tom lane

Re: minor fix of elevel in fd.c

From

"Qingqing Zhou"

Date:

13 June 2006, 01:58:53

"Tom Lane" <tgl@sss.pgh.pa.us> wrote
>
> > True, but current code just check the return value of AllocateABC() then
> > decide to upgrade elevel to FATAL.
>
> Where?
>

In ValidatePgVersion(), FindMyDatabase(), etc -- though in practice this
error can hardly happen anyway.

Regards,
Qingqing

Re: minor fix of elevel in fd.c

From

Tom Lane

Date:

13 June 2006, 02:09:16

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> "Tom Lane" <tgl@sss.pgh.pa.us> wrote
>>> True, but current code just check the return value of AllocateABC() then
>>> decide to upgrade elevel to FATAL.
>>
>> Where?

> In ValidatePgVersion(), FindMyDatabase(), etc -- though in practice this
> error can hardly happen anyway.

Doesn't matter because those functions only run during backend startup,
a time when ERROR and FATAL are equivalent anyhow.  It's debatable
whether those functions should be coded with the error level as ERROR
or FATAL --- there's no functional difference but it seems marginally
clearer to me to write FATAL in places where we know an error is going
to force backend exit.  You might see it differently though.

            regards, tom lane