Horton

Thread

Commits

  1. Remove STATUS_WAITING

  2. Remove STATUS_FOUND

  1. remove some STATUS_* symbols

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-12-29T10:33:34Z

    I have found the collection of STATUS_* defines in c.h a bit curious. 
There used to be a lot more even that have been removed over time. 
Currently, STATUS_FOUND and STATUS_WAITING are only used in one group of 
functions each, so perhaps it would make more sense to remove these from 
the global namespace and make them a local concern.

Attached are two patches to remove these two symbols.  STATUS_FOUND can 
be replaced by a simple bool.  STATUS_WAITING is replaced by a separate 
enum.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. Re: remove some STATUS_* symbols

    Michael Paquier <michael@paquier.xyz> — 2020-01-06T06:31:59Z

    On Sun, Dec 29, 2019 at 11:33:34AM +0100, Peter Eisentraut wrote:
> Attached are two patches to remove these two symbols.  STATUS_FOUND can be
> replaced by a simple bool.  STATUS_WAITING is replaced by a separate enum.

Patch 0001 looks good to me, but I got to wonder why the check after
waitMask in LockAcquireExtended() is not done directly in
LockCheckConflicts().

Regarding patch 0002, I am not sure that the addition of
ProcWaitStatus brings much though in terms of code readability.
--
Michael

  3. Re: remove some STATUS_* symbols

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-01-09T10:15:08Z

    On 2020-01-06 07:31, Michael Paquier wrote:
> On Sun, Dec 29, 2019 at 11:33:34AM +0100, Peter Eisentraut wrote:
>> Attached are two patches to remove these two symbols.  STATUS_FOUND can be
>> replaced by a simple bool.  STATUS_WAITING is replaced by a separate enum.
> 
> Patch 0001 looks good to me, but I got to wonder why the check after
> waitMask in LockAcquireExtended() is not done directly in
> LockCheckConflicts().

You mean put he subsequent GrantLock() calls into LockCheckConflicts()? 
That would technically save some duplicate code, but it seems weird, 
because LockCheckConflicts() is notionally a read-only function that 
shouldn't change state.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: remove some STATUS_* symbols

    Michael Paquier <michael@paquier.xyz> — 2020-01-10T05:23:32Z

    On Thu, Jan 09, 2020 at 11:15:08AM +0100, Peter Eisentraut wrote:
> You mean put he subsequent GrantLock() calls into LockCheckConflicts()? That
> would technically save some duplicate code, but it seems weird, because
> LockCheckConflicts() is notionally a read-only function that shouldn't
> change state.

Nah.  I was thinking about the first part of this "if" clause
LockCheckConflicts is part of here:
   if (lockMethodTable->conflictTab[lockmode] & lock->waitMask)
       status = STATUS_FOUND;
   else
       status = LockCheckConflicts(lockMethodTable, lockmode,
                                   lock, proclock);

But now that I look at it closely it messes up heavily with
ProcSleep() ;)
--
Michael

  5. Re: remove some STATUS_* symbols

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-01-11T07:14:17Z

    On 2020-01-10 06:23, Michael Paquier wrote:
> On Thu, Jan 09, 2020 at 11:15:08AM +0100, Peter Eisentraut wrote:
>> You mean put he subsequent GrantLock() calls into LockCheckConflicts()? That
>> would technically save some duplicate code, but it seems weird, because
>> LockCheckConflicts() is notionally a read-only function that shouldn't
>> change state.
> 
> Nah.  I was thinking about the first part of this "if" clause
> LockCheckConflicts is part of here:
>     if (lockMethodTable->conflictTab[lockmode] & lock->waitMask)
>         status = STATUS_FOUND;
>     else
>         status = LockCheckConflicts(lockMethodTable, lockmode,
>                                     lock, proclock);
> 
> But now that I look at it closely it messes up heavily with
> ProcSleep() ;)

OK, pushed as it was then.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: remove some STATUS_* symbols

    Michael Paquier <michael@paquier.xyz> — 2020-01-16T05:50:01Z

    On Sat, Jan 11, 2020 at 08:14:17AM +0100, Peter Eisentraut wrote:
> OK, pushed as it was then.

Thanks, that looks fine.  I am still not sure whether the second patch
adding an enum via ProcWaitStatus improves the code readability
though, so my take would be to discard it for now.  Perhaps others
think differently, I don't know.
--
Michael

  7. Re: remove some STATUS_* symbols

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-01-16T10:35:48Z

    At Thu, 16 Jan 2020 14:50:01 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Sat, Jan 11, 2020 at 08:14:17AM +0100, Peter Eisentraut wrote:
> > OK, pushed as it was then.
> 
> Thanks, that looks fine.  I am still not sure whether the second patch
> adding an enum via ProcWaitStatus improves the code readability
> though, so my take would be to discard it for now.  Perhaps others
> think differently, I don't know.

I feel the same about the second patch.

Although actually STATUS_WAITING is used only by ProcSleep and related
functions, likewise STATUS_EOF is seen only in auth.c/h. Other files,
pqcomm.c, crypt.c postmaster.c, hba.c, fe-auth.c , fe-connect.c,
fe-gssapi-common.c are using only STATUS_OK and ERROR. I haven't had a
close look but all of the usages would be equivalent to bool.

On the other hand many functions in fe-*.c and pqcomm.c returns
EOF(-1)/0 instead of STATUS_EOF(-2)/STATUS_OK(0).

We could reorganize the values and their usage but it doesn't seem to
be a big win..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  8. Re: remove some STATUS_* symbols

    Robert Haas <robertmhaas@gmail.com> — 2020-01-16T12:56:38Z

    On Thu, Jan 16, 2020 at 12:50 AM Michael Paquier <michael@paquier.xyz> wrote:
> Thanks, that looks fine.  I am still not sure whether the second patch
> adding an enum via ProcWaitStatus improves the code readability
> though, so my take would be to discard it for now.  Perhaps others
> think differently, I don't know.

IMHO, custom enums for each particular case would be a big improvement
over supposedly-generic STATUS codes. It makes it clearer which values
are possible in each code path, and it comes out nicer in the
debugger, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: remove some STATUS_* symbols

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-11T13:55:59Z

    On 2020-01-16 13:56, Robert Haas wrote:
> On Thu, Jan 16, 2020 at 12:50 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Thanks, that looks fine.  I am still not sure whether the second patch
>> adding an enum via ProcWaitStatus improves the code readability
>> though, so my take would be to discard it for now.  Perhaps others
>> think differently, I don't know.
> 
> IMHO, custom enums for each particular case would be a big improvement
> over supposedly-generic STATUS codes. It makes it clearer which values
> are possible in each code path, and it comes out nicer in the
> debugger, too.

Given this feedback, I would like to re-propose the original patch, 
attached again here.

After this, the use of the remaining STATUS_* symbols will be contained 
to the frontend and backend libpq code, so it'll be more coherent.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  10. Re: remove some STATUS_* symbols

    Michael Paquier <michael@paquier.xyz> — 2020-06-12T07:30:45Z

    On Thu, Jun 11, 2020 at 03:55:59PM +0200, Peter Eisentraut wrote:
> On 2020-01-16 13:56, Robert Haas wrote:
>> IMHO, custom enums for each particular case would be a big improvement
>> over supposedly-generic STATUS codes. It makes it clearer which values
>> are possible in each code path, and it comes out nicer in the
>> debugger, too.
> 
> Given this feedback, I would like to re-propose the original patch, attached
> again here.
> 
> After this, the use of the remaining STATUS_* symbols will be contained to
> the frontend and backend libpq code, so it'll be more coherent.

I am still in a so-so state regarding this patch, but I find the
debugger argument a good one.  And please don't consider me as a
blocker.

> Add a separate enum for use in the locking APIs, which were the only
> user.

> +typedef enum
> +{
> +	PROC_WAIT_STATUS_OK,
> +	PROC_WAIT_STATUS_WAITING,
> +	PROC_WAIT_STATUS_ERROR,
> +} ProcWaitStatus;

ProcWaitStatus, and more particularly PROC_WAIT_STATUS_WAITING are
strange names (the latter refers to "wait" twice).  What do you think
about renaming the enum to ProcStatus and the flags to PROC_STATUS_*?
--
Michael

  11. Re: remove some STATUS_* symbols

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-17T08:18:19Z

    On 2020-06-12 09:30, Michael Paquier wrote:
> On Thu, Jun 11, 2020 at 03:55:59PM +0200, Peter Eisentraut wrote:
>> On 2020-01-16 13:56, Robert Haas wrote:
>>> IMHO, custom enums for each particular case would be a big improvement
>>> over supposedly-generic STATUS codes. It makes it clearer which values
>>> are possible in each code path, and it comes out nicer in the
>>> debugger, too.
>>
>> Given this feedback, I would like to re-propose the original patch, attached
>> again here.
>>
>> After this, the use of the remaining STATUS_* symbols will be contained to
>> the frontend and backend libpq code, so it'll be more coherent.
> 
> I am still in a so-so state regarding this patch, but I find the
> debugger argument a good one.  And please don't consider me as a
> blocker.

Okay, I have committed it.

>> Add a separate enum for use in the locking APIs, which were the only
>> user.
> 
>> +typedef enum
>> +{
>> +	PROC_WAIT_STATUS_OK,
>> +	PROC_WAIT_STATUS_WAITING,
>> +	PROC_WAIT_STATUS_ERROR,
>> +} ProcWaitStatus;
> 
> ProcWaitStatus, and more particularly PROC_WAIT_STATUS_WAITING are
> strange names (the latter refers to "wait" twice).  What do you think
> about renaming the enum to ProcStatus and the flags to PROC_STATUS_*?

I see your point, but I don't think that's better.  That would just 
invite someone else to use it for other process-related status things. 
We typically name enum constants like the type followed by a suffix. 
The fact that the suffix is similar to the prefix here is more of a 
coincidence.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services