Обсуждение: remove some STATUS_* symbols
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
Вложения
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
Вложения
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
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
Вложения
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
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
Вложения
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
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
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
Вложения
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
Вложения
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