remove some STATUS_* symbols

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