Thread: [PATCH] Renumber confusing value for GUC_UNIT_BYTE
The GUC units are currently defined like: #define GUC_UNIT_KB 0x1000 /* value is in kilobytes */ #define GUC_UNIT_BLOCKS 0x2000 /* value is in blocks */ #define GUC_UNIT_XBLOCKS 0x3000 /* value is in xlog blocks */ #define GUC_UNIT_MB 0x4000 /* value is in megabytes */ #define GUC_UNIT_BYTE 0x8000 /* value is in bytes */ #define GUC_UNIT_MEMORY 0xF000 /* mask for size-related units */ #define GUC_UNIT_MS 0x10000 /* value is in milliseconds */ #define GUC_UNIT_S 0x20000 /* value is in seconds */ #define GUC_UNIT_MIN 0x30000 /* value is in minutes */ #define GUC_UNIT_TIME 0xF0000 /* mask for time-related units */ 0x3000 and 0x30000 seemed wrong, since they're a combination of other flags rather than being an independant power of two. But actually, these aren't flags: they're tested in a "case" statement for equality, not in a bitwise & test. So the outlier is actually 0x8000, added at: |commit 6e7baa322773ff8c79d4d8883c99fdeff5bfa679 |Author: Andres Freund <andres@anarazel.de> |Date: Tue Sep 12 12:13:12 2017 -0700 | | Introduce BYTES unit for GUCs. It looks like that originated here: https://www.postgresql.org/message-id/CAOG9ApEu8bXVwBxkOO9J7ZpM76TASK_vFMEEiCEjwhMmSLiaqQ%40mail.gmail.com commit 162e4838103e7957cccfe7868fc28397b55ca1d7 Author: Justin Pryzby <pryzbyj@telsasoft.com> Date: Wed Jul 20 09:27:24 2022 -0500 Renumber confusing value for GUC_UNIT_BYTE It had a power-of-two value, which looks right, and causes the other values which aren't powers-of-two to look wrong. But this is tested for equality and not a bitwise test. See also: 6e7baa322773ff8c79d4d8883c99fdeff5bfa679 https://www.postgresql.org/message-id/CAOG9ApEu8bXVwBxkOO9J7ZpM76TASK_vFMEEiCEjwhMmSLiaqQ%40mail.gmail.com diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index 4d0920c42e2..be928fac881 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -219,11 +219,12 @@ typedef enum #define GUC_DISALLOW_IN_AUTO_FILE 0x0800 /* can't set in * PG_AUTOCONF_FILENAME */ +/* GUC_UNIT_* are not flags - they're tested for equality */ #define GUC_UNIT_KB 0x1000 /* value is in kilobytes */ #define GUC_UNIT_BLOCKS 0x2000 /* value is in blocks */ #define GUC_UNIT_XBLOCKS 0x3000 /* value is in xlog blocks */ #define GUC_UNIT_MB 0x4000 /* value is in megabytes */ -#define GUC_UNIT_BYTE 0x8000 /* value is in bytes */ +#define GUC_UNIT_BYTE 0x5000 /* value is in bytes */ #define GUC_UNIT_MEMORY 0xF000 /* mask for size-related units */ #define GUC_UNIT_MS 0x10000 /* value is in milliseconds */
On 20.07.22 16:52, Justin Pryzby wrote: > +/* GUC_UNIT_* are not flags - they're tested for equality */ Well, there is GUC_UNIT_MEMORY, etc. so there is an additional constraint beyond just "pick any number". I'm not sure that "flag" and "tested for equality" are really antonyms anyway. I think renumbering this makes sense. We could just leave the comment as is if we don't come up with a better wording. > #define GUC_UNIT_KB 0x1000 /* value is in kilobytes */ > #define GUC_UNIT_BLOCKS 0x2000 /* value is in blocks */ > #define GUC_UNIT_XBLOCKS 0x3000 /* value is in xlog blocks */ > #define GUC_UNIT_MB 0x4000 /* value is in megabytes */ > -#define GUC_UNIT_BYTE 0x8000 /* value is in bytes */ > +#define GUC_UNIT_BYTE 0x5000 /* value is in bytes */ > #define GUC_UNIT_MEMORY 0xF000 /* mask for size-related units */ > > #define GUC_UNIT_MS 0x10000 /* value is in milliseconds */ > >
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> I think renumbering this makes sense. We could just leave the comment
> as is if we don't come up with a better wording.
+1, I see no need to change the comment. We just need to establish
the precedent that values within the GUC_UNIT_MEMORY field can be
chosen sequentially.
regards, tom lane
On Tue, Sep 06, 2022 at 01:57:53AM -0400, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes: >> I think renumbering this makes sense. We could just leave the comment >> as is if we don't come up with a better wording. > > +1, I see no need to change the comment. We just need to establish > the precedent that values within the GUC_UNIT_MEMORY field can be > chosen sequentially. +1. -- Michael
Attachment
On 06.09.22 08:27, Michael Paquier wrote: > On Tue, Sep 06, 2022 at 01:57:53AM -0400, Tom Lane wrote: >> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes: >>> I think renumbering this makes sense. We could just leave the comment >>> as is if we don't come up with a better wording. >> >> +1, I see no need to change the comment. We just need to establish >> the precedent that values within the GUC_UNIT_MEMORY field can be >> chosen sequentially. > > +1. committed without the comment change