Re: Remove dependence on integer wrapping
Nathan Bossart writes: > On Sun, Jun 09, 2024 at 09:57:54PM -0400, Tom Lane wrote: >> I think you could replace the whole thing by using overflow-aware >> multiplication and addition primitives in the result calculation. > I was still confused by the comment about 1999, but I tracked it down to > commit 542eeba [0]. IIUC it literally means that we need special handling > for that date because POSTGRES_EPOCH_JDATE is 2000-01-01. Yeah, I think so, and I think we probably don't need any special care if we switch to direct tests of overflow-aware primitives. (Though it'd be worth checking that '1999-12-31 24:00:00'::timestamp still works. It doesn't look like I actually added a test case for that.) regards, tom lane
Re: Remove dependence on integer wrapping
On Sun, Jun 09, 2024 at 09:57:54PM -0400, Tom Lane wrote: > Nathan Bossart writes: >> On Sun, Jun 09, 2024 at 04:59:22PM -0400, Joseph Koshakow wrote: >>> The following comment was in the code for parsing timestamps: >>> /* check for just-barely overflow (okay except time-of-day wraps) */ >>> /* caution: we want to allow 1999-12-31 24:00:00 */ >>> >>> I wasn't able to fully understand it even after staring at it for >>> a while. Is the comment suggesting that it is ok for the months field, >>> for example, to wrap around? That doesn't sound right to me I tested >>> the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same >>> before and after the patch. > >> I haven't stared at this for a while like you, but I am likewise confused >> at first glance. This dates back to commit 84df54b, and it looks like this >> comment was present in the first version of the patch in the thread [0]. I >> CTRL+F'd for any discussion about this but couldn't immediately find >> anything. > > I believe this is a copy-and-paste from 841b4a2d5, which added this: > > + *result = (date * INT64CONST(864)) + time; > + /* check for major overflow */ > + if ((*result - time) / INT64CONST(864) != date) > + return -1; > + /* check for just-barely overflow (okay except time-of-day wraps) */ > + if ((*result < 0) ? (date >= 0) : (date < 0)) > + return -1; > > I think you could replace the whole thing by using overflow-aware > multiplication and addition primitives in the result calculation. > Lines 2-4 basically check for mult overflow and 5-7 for addition > overflow. Ah, I see. Joe's patch does that in one place. It's probably worth doing that in the other places this "just-barefly overflow" comment appears IMHO. I was still confused by the comment about 1999, but I tracked it down to commit 542eeba [0]. IIUC it literally means that we need special handling for that date because POSTGRES_EPOCH_JDATE is 2000-01-01. [0] https://postgr.es/m/CABUevEx5zUO%3DKRUg06a9qnQ_e9EvTKscL6HxAM_L3xj71R7AQw%40mail.gmail.com -- nathan
Re: Assert in heapgettup_pagemode() fails due to underlying buffer change
I adjusted the code a bit more to look like the 16 coding including restoring some very useful comments that had been lost, and pushed. Thanks very much to Alexander and Noah for (independently) chasing this down and reporting, testing etc, and Alvaro and Robert for review comments. (Passing thought: it's a bit weird that we need to zero pages at all before restoring FPIs or initialising them. Perhaps there is some way to defer marking the buffer valid until after the caller gets a chance to initialise? Or something like that...)
Re: CheckMyDatabase some error messages in two lines.
Nathan Bossart writes:
> On Mon, Jun 10, 2024 at 08:00:00AM +0800, jian he wrote:
>> - errdetail("The database was initialized with
>> LC_COLLATE \"%s\", "
>> - " which is not recognized by
>> setlocale().", collate),
>> + errdetail("The database was initialized with
>> LC_COLLATE \"%s\", which is not recognized by setlocale().", collate),
> Both approaches produce the same message. With the existing code, the two
> string literals will be concatenated without newlines. It is probably
> split into two lines to avoid a long line in the source code.
No doubt. People have done it both ways in the past, but I think
currently there's a weak consensus in favor of using one line for
such messages even when it runs past 80 columns, mainly because
that makes it easier to grep the source code for a message text.
But: I don't see too much value in changing this particular instance,
because the line break is in a place where it would not likely cause
you to miss finding the line. You might grep for the first part of
the string or the second part, but probably not for ", which is not".
If the line break were in the middle of a phrase, there'd be more
argument for collapsing it out.
regards, tom lane
Re: CheckMyDatabase some error messages in two lines.
On Mon, Jun 10, 2024 at 08:00:00AM +0800, jian he wrote:
> https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-FORMATTING
> "Don't end a message with a newline."
>
>
> accidentally, I found some error messages in the function
> CheckMyDatabase spread into two lines.
> so i try to consolidate them into one line.
> - errdetail("The database was initialized with
> LC_COLLATE \"%s\", "
> -" which is not recognized by
> setlocale().", collate),
> + errdetail("The database was initialized with
> LC_COLLATE \"%s\", which is not recognized by setlocale().", collate),
Both approaches produce the same message. With the existing code, the two
string literals will be concatenated without newlines. It is probably
split into two lines to avoid a long line in the source code.
--
nathan
Re: Remove dependence on integer wrapping
Nathan Bossart writes: > On Sun, Jun 09, 2024 at 04:59:22PM -0400, Joseph Koshakow wrote: >> The following comment was in the code for parsing timestamps: >> /* check for just-barely overflow (okay except time-of-day wraps) */ >> /* caution: we want to allow 1999-12-31 24:00:00 */ >> >> I wasn't able to fully understand it even after staring at it for >> a while. Is the comment suggesting that it is ok for the months field, >> for example, to wrap around? That doesn't sound right to me I tested >> the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same >> before and after the patch. > I haven't stared at this for a while like you, but I am likewise confused > at first glance. This dates back to commit 84df54b, and it looks like this > comment was present in the first version of the patch in the thread [0]. I > CTRL+F'd for any discussion about this but couldn't immediately find > anything. I believe this is a copy-and-paste from 841b4a2d5, which added this: + *result = (date * INT64CONST(864)) + time; + /* check for major overflow */ + if ((*result - time) / INT64CONST(864) != date) + return -1; + /* check for just-barely overflow (okay except time-of-day wraps) */ + if ((*result < 0) ? (date >= 0) : (date < 0)) + return -1; I think you could replace the whole thing by using overflow-aware multiplication and addition primitives in the result calculation. Lines 2-4 basically check for mult overflow and 5-7 for addition overflow. BTW, while I approve of trying to get rid of our need for -fwrapv, I'm quite scared of actually doing it. Whatever cases you may have discovered by running the regression tests will be at best the tip of the iceberg. Is there any chance of using static analysis to find all the places of concern? regards, tom lane
Re: Remove dependence on integer wrapping
On Sun, Jun 09, 2024 at 04:59:22PM -0400, Joseph Koshakow wrote: > I originally added the helper functions to int.h thinking I'd find > many more instances of overflow due to integer negation, however I > didn't find that many. So let me know if you think we'd be better > off without the functions. I'd vote for the functions, even if it's just future-proofing at the moment. IMHO it helps with readability, too. > The following comment was in the code for parsing timestamps: > > /* check for just-barely overflow (okay except time-of-day wraps) */ > /* caution: we want to allow 1999-12-31 24:00:00 */ > > I wasn't able to fully understand it even after staring at it for > a while. Is the comment suggesting that it is ok for the months field, > for example, to wrap around? That doesn't sound right to me I tested > the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same > before and after the patch. I haven't stared at this for a while like you, but I am likewise confused at first glance. This dates back to commit 84df54b, and it looks like this comment was present in the first version of the patch in the thread [0]. I CTRL+F'd for any discussion about this but couldn't immediately find anything. > /* check the negative equivalent will fit without overflowing */ > if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)) > goto out_of_range; > + > + /* > + * special case the minimum integer because its negation cannot > be > + * represented > + */ > + if (tmp == ((uint16) PG_INT16_MAX) + 1) > + return PG_INT16_MIN; > return -((int16) tmp); My first impression is that there appears to be two overflow checks, one of which sends us to out_of_range, and another that just returns a special result. Why shouldn't we add a pg_neg_s16_overflow() and replace this whole chunk with something like this? if (unlikely(pg_neg_s16_overflow(tmp, ))) goto out_of_range; else return tmp; > + return ((uint32) INT32_MAX) + 1; > + return ((uint64) INT64_MAX) + 1; nitpick: Any reason not to use PG_INT32_MAX/PG_INT64_MAX for these? [0] https://postgr.es/m/flat/CAFj8pRBwqALkzc%3D1WV%2Bh5e%2BDcALY2EizjHCvFi9vHbs%2Bz1OhjA%40mail.gmail.com -- nathan
CheckMyDatabase some error messages in two lines.
hi.
https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-FORMATTING
"Don't end a message with a newline."
accidentally, I found some error messages in the function
CheckMyDatabase spread into two lines.
so i try to consolidate them into one line.
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 0805398e..d73343e4 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -408,15 +408,13 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
if (pg_perm_setlocale(LC_COLLATE, collate) == NULL)
ereport(FATAL,
(errmsg("database locale is incompatible with operating system"),
- errdetail("The database was initialized with LC_COLLATE \"%s\", "
- " which is not recognized by setlocale().", collate),
+ errdetail("The database was initialized with LC_COLLATE \"%s\", which is not recognized by setlocale().", collate),
errhint("Recreate the database with another locale or install the missing locale.")));
if (pg_perm_setlocale(LC_CTYPE, ctype) == NULL)
ereport(FATAL,
(errmsg("database locale is incompatible with operating system"),
- errdetail("The database was initialized with LC_CTYPE \"%s\", "
- " which is not recognized by setlocale().", ctype),
+ errdetail("The database was initialized with LC_CTYPE \"%s\", which is not recognized by setlocale().", ctype),
errhint("Recreate the database with another locale or install the missing locale.")));
if (strcmp(ctype, "C") == 0 ||
@@ -490,12 +488,9 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
ereport(WARNING,
(errmsg("database \"%s\" has a collation version mismatch",
name),
- errdetail("The database was created using collation version %s, "
- "but the operating system provides version %s.",
+ errdetail("The database was created using collation version %s, but the operating system provides version %s.",
collversionstr, actual_versionstr),
- errhint("Rebuild all objects in this database that use the default collation and run "
- "ALTER DATABASE %s REFRESH COLLATION VERSION, "
- "or build PostgreSQL with the right library version.",
+ errhint("Rebuild all objects in this database that use the default collation and run ALTER DATABASE %s REFRESH COLLATION VERSION or build PostgreSQL with the right library version.",
quote_identifier(name;
}
Remove dependence on integer wrapping
Hi,
In [0] Andres suggested enabling -ftrapv in assert enabled builds. While
I vastly underestimated the complexity of updating `configure` to do
this, I was able to enable the flag locally. Enabling this flag causes
our existing regression tests to trap and fail in multiple different
spots. The attached patch resolves all of these overflows so that all
of our existing tests will pass with the -ftrapv flag enabled.
Some notes on the patch itself are:
I originally added the helper functions to int.h thinking I'd find
many more instances of overflow due to integer negation, however I
didn't find that many. So let me know if you think we'd be better
off without the functions.
I considered using #ifdef to rely on wrapping when -fwrapv was
enabled. This would save us some unnecessary branching when we could
rely on wrapping behavior, but it would mean that we could only enable
-ftrapv when -fwrapv was disabled, greatly reducing its utility.
The following comment was in the code for parsing timestamps:
/* check for just-barely overflow (okay except time-of-day wraps) */
/* caution: we want to allow 1999-12-31 24:00:00 */
I wasn't able to fully understand it even after staring at it for
a while. Is the comment suggesting that it is ok for the months field,
for example, to wrap around? That doesn't sound right to me I tested
the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same
before and after the patch.
Thanks,
Joe Koshakow
[0]
https://www.postgresql.org/message-id/20240213191401.jjhsic7et4tiahjs%40awork3.anarazel.de
From 319bc904858ad8fbcc687a923733defd3358c7b9 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH] Remove dependence on integer wrapping
This commit updates various parts of the code to no longer rely on
integer wrapping for correctness. Not all compilers support -fwrapv, so
it's best not to rely on it.
---
src/backend/utils/adt/cash.c | 7 +++--
src/backend/utils/adt/numeric.c | 5 ++--
src/backend/utils/adt/numutils.c | 35 ++
src/backend/utils/adt/timestamp.c | 13 ++---
src/include/common/int.h | 48 +++
5 files changed, 92 insertions(+), 16 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..f6f095a57b 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -352,8 +352,11 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
+ /*
+ * make the amount positive for digit-reconstruction loop, we can
+ * leave INT64_MIN unchanged
+ */
+ pg_neg_s64_overflow(value, );
/* set up formatting data */
signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
sign_posn = lconvert->n_sign_posn;
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 5510a203b0..4ea2d9b0b4 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8110,15 +8110,14 @@ int64_to_numericvar(int64 val, NumericVar *var)
/* int64 can require at most 19 decimal digits; add one for safety */
alloc_var(var, 20 / DEC_DIGITS);
+ uval = pg_abs_s64(val);
if (val < 0)
{
var->sign = NUMERIC_NEG;
- uval = -val;
}
else
{
var->sign = NUMERIC_POS;
- uval = val;
}
var->dscale = 0;
if (val == 0)
@@ -11222,7 +11221,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
* Now we can proceed with the multiplications.
*/
neg = (exp < 0);
- mask = abs(exp);
+ mask = pg_abs_s32(exp);
init_var(_prod);
set_var_from_var(base, _prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..12bef9d63c 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -193,6 +193,13 @@ pg_strtoint16_safe(const char *s, Node *escontext)
/* check the negative equivalent will fit without overflowing */
if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint16) PG_INT16_MAX) + 1)
+ return PG_INT16_MIN;
return -((int16) tmp);
}
@@ -336,6 +343,13 @@ slow:
/* check the negative equivalent will fit without overflowing */
if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint16) PG_INT16_MAX) + 1)
+ return PG_INT16_MIN;
return -((int16) tmp);
}
@@ -598,6 +612,13 @@ slow:
/* check the negative equivalent will fit without overflowing */
if (tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1)
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint32)
Re: Wrong security context for deferred triggers?
On Sat, Jun 8, 2024 at 10:13 PM Isaac Morland wrote: > Speaking as a table owner, when I set a trigger on it, I expect that when the specified actions occur my trigger will fire and will do what I specify, without regard to the execution environment of the caller (search_path in particular); and my trigger should be able to do anything that I can do. For the canonical case of a logging table the trigger has to be able to do stuff the caller can't do. I don't expect to be able to do stuff that the caller can do. > > Speaking as someone making an update on a table, I don't expect to have it fail because my execution environment (search_path in particular) is wrong for the trigger implementation, and I consider it a security violation if the table owner is able to do stuff as me as a result, especially if I am an administrator making an update as superuser. Can you expand on this a bit? When a trigger executes should the execution environment match: - The execution environment of the trigger owner at the time of trigger creation? - The execution environment of the function owner at the time of function creation? - An execution environment built from the trigger owner's default configuration parameters? - Something else? While I am convinced that privileges should be checked using the trigger owner's role, I'm less convinced of other configuration parameters. For the search_path example, that can be resolved by either fully qualifying object names or setting the search_path in the function itself. Similar approaches can be taken with other configuration parameters. I also worry that it would be a source of confusion that the execution environment of triggers come from the trigger/function owner, but the execution environment of function calls come from the caller. > I think it's pretty clear the existing behaviour is the wrong choice in every other way than backward compatibility. I welcome examples to the contrary, where the existing behaviour is not just OK but actually wanted. This is perhaps a contrived example, but here's one. Suppose I create a trigger that raises a notice that includes the current timestamp. I would probably want to use the timezone of the caller, not the trigger owner. Thanks, Joe Koshakow
Re: problems with "Shared Memory and Semaphores" section of docs
On Thu, Jun 06, 2024 at 02:51:42PM -0500, Nathan Bossart wrote: > On Thu, Jun 06, 2024 at 03:31:53PM -0400, Robert Haas wrote: >> I do think the name could use some more thought, though. >> semaphores_required would end up being the same kind of thing as >> shared_memory_size_in_huge_pages, but the names seem randomly >> different. If semaphores_required is right here, why isn't >> shared_memory_required used there? Seems more like we ought to call >> this semaphores or os_semaphores or num_semaphores or >> num_os_semaphores or something. > > I'm fine with any of your suggestions. If I _had_ to pick one, I'd > probably choose num_os_semaphores because it's the most descriptive. Here's a new version of the patch with the GUC renamed to num_os_semaphores. -- nathan >From 4321c19071eec065d8712ae0080d237d56a1478e Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Tue, 21 May 2024 14:02:22 -0500 Subject: [PATCH v3 1/1] add num_os_semaphores GUC --- doc/src/sgml/config.sgml| 14 +++ doc/src/sgml/runtime.sgml | 39 - src/backend/storage/ipc/ipci.c | 6 - src/backend/utils/misc/guc_tables.c | 12 + 4 files changed, 53 insertions(+), 18 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 698169afdb..a8372eadf5 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -11215,6 +11215,20 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' + + num_os_semaphores (integer) + + num_os_semaphores configuration parameter + + + + +Reports the number of semaphores that are needed for the server based +on the number of allowed connections, worker processes, etc. + + + + ssl_library (string) diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml index 2f7c618886..26f3cbe555 100644 --- a/doc/src/sgml/runtime.sgml +++ b/doc/src/sgml/runtime.sgml @@ -781,13 +781,13 @@ psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: No such SEMMNI Maximum number of semaphore identifiers (i.e., sets) -at least ceil((max_connections + autovacuum_max_workers + max_wal_senders + max_worker_processes + 7) / 16) plus room for other applications +at least ceil(num_os_semaphores / 16) plus room for other applications SEMMNS Maximum number of semaphores system-wide -ceil((max_connections + autovacuum_max_workers + max_wal_senders + max_worker_processes + 7) / 16) * 17 plus room for other applications +ceil(num_os_semaphores / 16) * 17 plus room for other applications @@ -836,30 +836,38 @@ psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: No such When using System V semaphores, -PostgreSQL uses one semaphore per allowed connection -(), allowed autovacuum worker process -(), allowed WAL sender process -(), and allowed background -process (), in sets of 16. +PostgreSQL uses one semaphore per allowed connection, +worker process, etc., in sets of 16. Each such set will also contain a 17th semaphore which contains a magic number, to detect collision with semaphore sets used by other applications. The maximum number of semaphores in the system is set by SEMMNS, which consequently must be at least -as high as max_connections plus -autovacuum_max_workers plus max_wal_senders, -plus max_worker_processes, plus one extra for each 16 -allowed connections plus workers (see the formula in plus one extra for +each set of 16 required semaphores (see the formula in ). The parameter SEMMNI determines the limit on the number of semaphore sets that can exist on the system at one time. Hence this parameter must be at -least ceil((max_connections + autovacuum_max_workers + max_wal_senders + max_worker_processes + 7) / 16). +least ceil(num_os_semaphores / 16). Lowering the number of allowed connections is a temporary workaround for failures, which are usually confusingly worded No space left on device, from the function semget. + +The number of semaphores required by PostgreSQL +is provided by the runtime-computed parameter +num_os_semaphores, which can be determined before +starting the server with a postgres command like: + +$ postgres -D $PGDATA -C num_os_semaphores + +The value of num_os_semaphores should be input into +the aforementioned formulas to determine appropriate values for +SEMMNI and SEMMNS. + + In some cases it might also be necessary to increase SEMMAP to be at least on the order of @@ -882,11 +890,8 @@ psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: No such When using POSIX semaphores, the
Re: Show WAL write and fsync stats in pg_stat_io
> If possible, let's have all the I/O stats (even for WAL) in > pg_stat_io. Can't we show the WAL data we get from buffers in the hits > column and then have read_bytes or something like that to know the > amount of data read? The ‘hits’ column in ‘pg_stat_io’ is a vital indicator for adjusting a database. It signifies the count of cache hits, or in other words, the instances where data was located in the ‘shared_buffers’. As a result, keeping an eye on the ‘hits’ column in ‘pg_stat_io’ can offer useful knowledge about the buffer cache’s efficiency and assist users in making educated choices when fine-tuning their database. However, if we include the hit count of WAL buffers in this, it may lead to misleading interpretations for database tuning. If there’s something I’ve overlooked that’s already been discussed, please feel free to correct me. Best Regards, Nitin Jadhav Azure Database for PostgreSQL Microsoft On Tue, May 28, 2024 at 6:18 AM Amit Kapila wrote: > > On Mon, May 13, 2024 at 7:42 PM Bharath Rupireddy > wrote: > > > > On Fri, Apr 19, 2024 at 1:32 PM Nazir Bilal Yavuz > > wrote: > > > > > > > I wanted to inform you that the 73f0a13266 commit changed all WALRead > > > > calls to read variable bytes, only the WAL receiver was reading > > > > variable bytes before. > > > > > > I want to start working on this again if possible. I will try to > > > summarize the current status: > > > > Thanks for working on this. > > > > > * With the 73f0a13266 commit, the WALRead() function started to read > > > variable bytes in every case. Before, only the WAL receiver was > > > reading variable bytes. > > > > > > * With the 91f2cae7a4 commit, WALReadFromBuffers() is merged. We were > > > discussing what we have to do when this is merged. It is decided that > > > WALReadFromBuffers() does not call pgstat_report_wait_start() because > > > this function does not perform any IO [1]. We may follow the same > > > logic by not including these to pg_stat_io? > > > > Right. WALReadFromBuffers doesn't do any I/O. > > > > Whoever reads WAL from disk (backends, walsenders, recovery process) > > using pg_pread (XLogPageRead, WALRead) needs to be tracked in > > pg_stat_io or some other view. If it were to be in pg_stat_io, > > although we may not be able to distinguish WAL read stats at a backend > > level (like how many times/bytes a walsender or recovery process or a > > backend read WAL from disk), but it can help understand overall impact > > of WAL read I/O at a cluster level. With this approach, the WAL I/O > > stats are divided up - WAL read I/O and write I/O stats are in > > pg_stat_io and pg_stat_wal respectively. > > > > This makes me think if we need to add WAL read I/O stats also to > > pg_stat_wal. Then, we can also add WALReadFromBuffers stats > > hits/misses there. With this approach, pg_stat_wal can be a one-stop > > view for all the WAL related stats. > > > > If possible, let's have all the I/O stats (even for WAL) in > pg_stat_io. Can't we show the WAL data we get from buffers in the hits > column and then have read_bytes or something like that to know the > amount of data read? > > -- > With Regards, > Amit Kapila. > >
Format the code in xact_decode
Hello hackers, just as the title says: 1.Remove redundant parentheses. 2.Adjust the position of the break statement. -- Regards, ChangAo Chen 0001-Format-the-code-in-xact_decode.patch Description: Binary data
Re: Fix grammar oddities in comments
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested Confirming linguistic correctness of changes, and lack of anything changed outside of comments that would otherwise affect readiness to commit. The new status of this patch is: Ready for Committer
The content of the column_name field in the error response for a constraint violation
Hi, I noticed that the error response for a constraint violation only contains the column name for not-null constraints. I'm confused because the field isn't present when other types of constraints are triggered, such as unique, foreign keys, and check constraints. Was this done intentionally because these other constraints may involve multiple columns, while the column_name field expects a single column? I understand that a client can find out which columns are involved by a constraint name. Alternatively, should it be made more handy so that the column_name field is present for all constraints and includes all involved columns? С уважением, Архипов Никита
Re: Windows: openssl & gssapi dislike each other
Hi
I am submitting two new patches. We can undefine the macro at two locations
1). As be-secure-openssl.c [1] was the actual
file where the conflict happened so I undefined the macro here before
the ssl includes. I changed the comment a little to make it understandable.
I am also attaching the error generated with ninja build.
OR
2). Right after the gssapi includes in libpq-be.h
Thanks
Imran Zaheer
Bitnine
[1]:
https://github.com/postgres/postgres/blob/00ac25a3c365004821e819653c3307acd3294818/src/backend/libpq/be-secure-openssl.c#L46
On Sun, Jun 9, 2024 at 7:21 AM Tom Lane wrote:
>
> Andrew Dunstan writes:
> > On 2024-06-08 Sa 06:22, Imran Zaheer wrote:
> >> Now this can either be solved by just just undefine the macro defined
> >> by wincrypt.h as done here [3]
> >> Or we should rearrange our headers. Openssl header should be at the
> >> bottom (after the gssapi includes).
>
> > Let's be consistent and use the #undef from [3].
>
> +1. Depending on header order is not great, especially when you have
> to make it depend on an order that is directly contradictory to
> project conventions [0].
>
> regards, tom lane
>
> [0] https://wiki.postgresql.org/wiki/Committing_checklist#Policies
Found ninja-1.11.1.git.kitware.jobserver-1 at
C:\Users\Imran\AppData\Local\Programs\Python\Python311\Scripts\ninja.EXE
ninja: Entering directory `build'
[439/2267] Linking target src/interfaces/libpq/libpq.dll
Creating library src/interfaces/libpq\libpq.lib
[841/2267] Compiling C object
src/backend/postgres_lib.a.p/libpq_be-secure-openssl.c.obj
FAILED: src/backend/postgres_lib.a.p/libpq_be-secure-openssl.c.obj
"C:\Program Files\Microsoft Visual
Studio\2022\Preview\VC\Tools\MSVC\14.39.33218\bin\Hostx64\x64\cl.exe"
"-Isrc\backend\postgres_lib.a.p" "-Isrc\include" "-I..\src\include"
"-I..\src\include\port\win32" "-I..\src\include\port\win32_msvc"
"-ID:\kbr\include" "-ID:\kbr\include\krb5" "-IC:/Program
Files/OpenSSL-Win64/include" "/MDd" "/FIpostgres_pch.h" "/Yupostgres_pch.h"
"/Fpsrc\backend\postgres_lib.a.p\postgres_pch.pch" "/nologo" "/showIncludes"
"/utf-8" "/W2" "/Od" "/Zi" "/DWIN32" "/DWINDOWS" "/D__WINDOWS__" "/D__WIN32__"
"/D_CRT_SECURE_NO_DEPRECATE" "/D_CRT_NONSTDC_NO_DEPRECATE" "/wd4018" "/wd4244"
"/wd4273" "/wd4101" "/wd4102" "/wd4090" "/wd4267" "-DBUILDING_DLL" "/FS"
"/FdC:\Users\Imran\Desktop\agens-tmp\compile\postgres\pg\postgres\build\src\backend\postgres_lib.pdb"
/Fosrc/backend/postgres_lib.a.p/libpq_be-secure-openssl.c.obj "/c"
../src/backend/libpq/be-secure-openssl.c
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(140): error C2059:
syntax error: '('
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(147): error C2059:
syntax error: ''
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(152): error C2059:
syntax error: '}'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(153): error C2059:
syntax error: '}'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(157): error C2061:
syntax error: identifier 'GENERAL_NAME'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(158): error C2059:
syntax error: '}'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2143:
syntax error: missing ')' before '*'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2143:
syntax error: missing '{' before '*'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): warning C4228:
nonstandard extension used: qualifiers after comma in declarator list are
ignored
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2143:
syntax error: missing ';' before '*'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2059:
syntax error: ')'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2373: 'a':
redefinition; different type modifiers
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): note: see
declaration of 'a'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2146:
syntax error: missing ')' before identifier 'compare'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2061:
syntax error: identifier 'compare'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2059:
syntax error: ';'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2449:
found '{' at file scope (missing function header?)
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2059:
syntax error: '}'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2059:
syntax error: ','
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2081:
'GENERAL_NAME': name in formal parameter list illegal
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2054:
expected '(' to follow 'ptr'
C:\Program Files\OpenSSL-Win64\include\openssl/x509v3.h(166): error C2146:
syntax error: missing ')' before identifier 'freefunc'
C:\Program
