Replace (GUC_UNIT_MEMORY | GUC_UNIT_TIME) with GUC_UNIT in guc.c
Hi, hackers
We use (GUC_UNIT_MEMORY | GUC_UNIT_TIME) instead of GUC_UNIT even though we
already define it in guc.h.
Maybe using GUC_UNIT is better? Here is a patch to fix it.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a9033b7a54..5308896c87 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2766,7 +2766,7 @@ convert_real_from_base_unit(double base_value, int base_unit,
const char *
get_config_unit_name(int flags)
{
- switch (flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME))
+ switch (flags & GUC_UNIT)
{
case 0:
return NULL; /* GUC has no units */
@@ -2802,7 +2802,7 @@ get_config_unit_name(int flags)
return "min";
default:
elog(ERROR, "unrecognized GUC units value: %d",
- flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME));
+ flags & GUC_UNIT);
return NULL;
}
}--
Regrads,
Japin Li.
On Wed, Jun 14, 2023 at 12:33 PM Japin Li <japinli@hotmail.com> wrote:
Hi, hackers
We use (GUC_UNIT_MEMORY | GUC_UNIT_TIME) instead of GUC_UNIT even though we
already define it in guc.h.Maybe using GUC_UNIT is better? Here is a patch to fix it.
Yeah, it seems more consistent with other places in guc.c. I'll push
it, barring any objections.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Wed, Jun 14, 2023 at 1:07 PM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:
On Wed, Jun 14, 2023 at 12:33 PM Japin Li <japinli@hotmail.com> wrote:
Hi, hackers
We use (GUC_UNIT_MEMORY | GUC_UNIT_TIME) instead of GUC_UNIT even though
we
already define it in guc.h.
Maybe using GUC_UNIT is better? Here is a patch to fix it.
Yeah, it seems more consistent with other places in guc.c. I'll push
it, barring any objections.
+1. BTW, it seems that GUC_UNIT_TIME is not used anywhere except in
GUC_UNIT. I was wondering if we can retire it, but maybe we'd better
not. It still indicates that we need to use time units table.
Thanks
Richard
On Wed, Jun 14, 2023 at 03:38:10PM +0800, Richard Guo wrote:
+1. BTW, it seems that GUC_UNIT_TIME is not used anywhere except in
GUC_UNIT. I was wondering if we can retire it, but maybe we'd better
not. It still indicates that we need to use time units table.
Some out-of-core code declaring custom GUCs could rely on that, so
it is better not to remove it.
--
Michael
On Wed, Jun 14, 2023 at 3:47 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 14, 2023 at 03:38:10PM +0800, Richard Guo wrote:
+1. BTW, it seems that GUC_UNIT_TIME is not used anywhere except in
GUC_UNIT. I was wondering if we can retire it, but maybe we'd better
not. It still indicates that we need to use time units table.Some out-of-core code declaring custom GUCs could rely on that, so
it is better not to remove it.
I see. Thanks for pointing that out.
Thanks
Richard
On Wed, 14 Jun 2023 at 17:52, Richard Guo <guofenglinux@gmail.com> wrote:
On Wed, Jun 14, 2023 at 3:47 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 14, 2023 at 03:38:10PM +0800, Richard Guo wrote:
+1. BTW, it seems that GUC_UNIT_TIME is not used anywhere except in
GUC_UNIT. I was wondering if we can retire it, but maybe we'd better
not. It still indicates that we need to use time units table.Some out-of-core code declaring custom GUCs could rely on that, so
it is better not to remove it.I see. Thanks for pointing that out.
Thanks for all of your reviews. Agreed with Michael do not touch GUC_UNIT_TIME.
--
Regrads,
Japin Li.
On Wed, Jun 14, 2023 at 4:47 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 14, 2023 at 03:38:10PM +0800, Richard Guo wrote:
+1. BTW, it seems that GUC_UNIT_TIME is not used anywhere except in
GUC_UNIT. I was wondering if we can retire it, but maybe we'd better
not. It still indicates that we need to use time units table.Some out-of-core code declaring custom GUCs could rely on that, so
it is better not to remove it.
+1 not to remove it.
I've attached the patch to fix (GUC_UNIT_MEMORY | GUC_UNIT_TIME)
thing, and am going to push it later today to only master branch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Replace-GUC_UNIT_MEMORY-GUC_UNIT_TIME-with-GUC_UN.patchapplication/octet-stream; name=v1-0001-Replace-GUC_UNIT_MEMORY-GUC_UNIT_TIME-with-GUC_UN.patchDownload
From 30782542ebaca71153d80fa3e67a68e54e4727ee Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <msawada@postgresql.orig>
Date: Thu, 15 Jun 2023 10:52:35 +0900
Subject: [PATCH v1] Replace GUC_UNIT_MEMORY|GUC_UNIT_TIME with GUC_UNIT.
We used (GUC_UNIT_MEMORY | GUC_UNIT_TIME) instead of GUC_UNIT some
places but we already define it in guc.h. This commit replaces them
with GUC_UNIT for better consistency with their surrounding code.
Author: Japin Li
Reviewed-by: Richard Guo, Michael Paquier, Masahiko Sawada
Discussion: https://postgr.es/m/MEYP282MB1669EC0FED922F7A151673ACB65AA@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
---
src/backend/utils/misc/guc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a9033b7a54..5308896c87 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2766,7 +2766,7 @@ convert_real_from_base_unit(double base_value, int base_unit,
const char *
get_config_unit_name(int flags)
{
- switch (flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME))
+ switch (flags & GUC_UNIT)
{
case 0:
return NULL; /* GUC has no units */
@@ -2802,7 +2802,7 @@ get_config_unit_name(int flags)
return "min";
default:
elog(ERROR, "unrecognized GUC units value: %d",
- flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME));
+ flags & GUC_UNIT);
return NULL;
}
}
--
2.31.1
On Thu, Jun 15, 2023 at 11:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Wed, Jun 14, 2023 at 4:47 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 14, 2023 at 03:38:10PM +0800, Richard Guo wrote:
+1. BTW, it seems that GUC_UNIT_TIME is not used anywhere except in
GUC_UNIT. I was wondering if we can retire it, but maybe we'd better
not. It still indicates that we need to use time units table.Some out-of-core code declaring custom GUCs could rely on that, so
it is better not to remove it.+1 not to remove it.
I've attached the patch to fix (GUC_UNIT_MEMORY | GUC_UNIT_TIME)
thing, and am going to push it later today to only master branch.
Pushed.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com