public inbox for [email protected]
help / color / mirror / Atom feedpg_walfile_name_offset can return inconsistent values
7+ messages / 6 participants
[nested] [flat]
* pg_walfile_name_offset can return inconsistent values
@ 2019-07-26 08:21 Kyotaro Horiguchi <[email protected]>
2019-07-26 09:30 ` Re: pg_walfile_name_offset can return inconsistent values Jehan-Guillaume de Rorthais <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Kyotaro Horiguchi @ 2019-07-26 08:21 UTC (permalink / raw)
To: [email protected]
Hello.
While looking [1], I noticed that pg_walfile_name_offset behaves
somewhat oddly at segment boundary.
select * from (values ('0/16ffffff'), ('0/17000000'), ('0/17000001')) as t(lsn), lateral pg_walfile_name_offset(lsn::pg_lsn);
lsn | file_name | file_offset
------------+--------------------------+-------------
0/16ffffff | 000000020000000000000016 | 16777215
0/17000000 | 000000020000000000000016 | 0
0/17000001 | 000000020000000000000017 | 1
The file names are right as defined, but the return value of the
second line wrong, or at least misleading. It should be (16,
1000000) or (16, FFFFFF). The former is out-of-domain so we would
have no way than choosing the latter. I'm not sure the purpose of
the second output parameter, thus the former might be right
decision.
The function returns the following result after this patch is
applied.
select * from (values ('0/16ffffff'), ('0/17000000'), ('0/17000001')) as t(lsn), lateral pg_walfile_name_offset(lsn::pg_lsn);
lsn | file_name | file_offset
------------+--------------------------+-------------
0/16ffffff | 000000020000000000000016 | 16777214
0/17000000 | 000000020000000000000016 | 16777215
0/17000001 | 000000020000000000000017 | 0
regards.
[1]: https://www.postgresql.org/message-id/20190725193808.1648ddc8@firost
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_walfile_name_offset can return inconsistent values
2019-07-26 08:21 pg_walfile_name_offset can return inconsistent values Kyotaro Horiguchi <[email protected]>
@ 2019-07-26 09:30 ` Jehan-Guillaume de Rorthais <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2019-07-26 09:30 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]
On Fri, 26 Jul 2019 17:21:20 +0900 (Tokyo Standard Time)
Kyotaro Horiguchi <[email protected]> wrote:
> Hello.
>
> While looking [1], I noticed that pg_walfile_name_offset behaves
> somewhat oddly at segment boundary.
>
> select * from (values ('0/16ffffff'), ('0/17000000'), ('0/17000001')) as
> t(lsn), lateral pg_walfile_name_offset(lsn::pg_lsn);
> lsn | file_name | file_offset
> ------------+--------------------------+-------------
> 0/16ffffff | 000000020000000000000016 | 16777215
> 0/17000000 | 000000020000000000000016 | 0
> 0/17000001 | 000000020000000000000017 | 1
>
>
> The file names are right as defined, but the return value of the
> second line wrong, or at least misleading.
+1
I noticed it as well and put this report on hold while working on my patch.
Thanks for reporting this!
> It should be (16, 1000000) or (16, FFFFFF). The former is out-of-domain so we
> would have no way than choosing the latter. I'm not sure the purpose of
> the second output parameter, thus the former might be right
> decision.
>
> The function returns the following result after this patch is
> applied.
>
> select * from (values ('0/16ffffff'), ('0/17000000'), ('0/17000001')) as
> t(lsn), lateral pg_walfile_name_offset(lsn::pg_lsn);
> lsn | file_name | file_offset
> ------------+--------------------------+-------------
> 0/16ffffff | 000000020000000000000016 | 16777214
> 0/17000000 | 000000020000000000000016 | 16777215
> 0/17000001 | 000000020000000000000017 | 0
So you shift the file offset for all LSN by one byte? This could lead to
regression in various tools relying on this function.
Moreover, it looks weird as the LSN doesn't reflect the given offset anymore
(FFFFFF <> 16777214, 000001 <> 0, etc).
Another solution might be to return the same result when for both 0/16ffffff and
0/17000000, but it doesn't feel right either.
So in fact, returning 0x1000000 seems to be the cleaner result to me.
Regards,
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH 07/16] +nondecimal
@ 2022-12-15 01:04 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Justin Pryzby @ 2022-12-15 01:04 UTC (permalink / raw)
Per Erik Rijkers
---
doc/src/sgml/syntax.sgml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 956182e7c6a..0ccddea310d 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -722,7 +722,7 @@ $function$
<note>
<para>
- Nondecimal integer constants are currently only supported in the range
+ Non-decimal integer constants are currently only supported in the range
of the <type>bigint</type> type (see <xref
linkend="datatype-numeric-table"/>).
</para>
--
2.25.1
--TBNym+cBXeFsS4Vs
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0008-do-not-end-messages-with-newlines.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix calloc check if oom (PQcancelCreate)
@ 2024-05-27 16:47 Jelte Fennema-Nio <[email protected]>
2024-05-27 17:28 ` Re: Fix calloc check if oom (PQcancelCreate) Ranier Vilela <[email protected]>
2024-05-27 17:42 ` Re: Fix calloc check if oom (PQcancelCreate) Daniel Gustafsson <[email protected]>
0 siblings, 2 replies; 7+ messages in thread
From: Jelte Fennema-Nio @ 2024-05-27 16:47 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers
On Mon, 27 May 2024 at 18:16, Ranier Vilela <[email protected]> wrote:
> Is it mandatory to call PQcancelFinish in case PQcancelCreate fails?
Yes, see the following line in the docs:
Note that when PQcancelCreate returns a non-null pointer, you must
call PQcancelFinish when you are finished with it, in order to dispose
of the structure and any associated memory blocks. This must be done
even if the cancel request failed or was abandoned.
Source: https://www.postgresql.org/docs/17/libpq-cancel.html#LIBPQ-PQCANCELCREATE
> IMO, I would expect problems with users.
This pattern is taken from regular connection creation and is really
common in libpq code so I don't expect issues. And even if there were
an issue, your v1 patch would not be nearly enough. Because you're
only freeing the connhost and addr field now in case of OOM. But there
are many more fields that would need to be freed.
>> And that function will free any
>> partially initialized PGconn correctly. This is also how
>> pqConnectOptions2 works.
>
> Well, I think that function *pqReleaseConnHost*, is incomplete.
> 1. Don't free connhost field;
ehm... it does free that afaict? It only doesn't set it to NULL. Which
indeed would be good to do, but not doing so doesn't cause any issues
with it's current 2 usages afaict.
> 2. Don't free addr field;
That's what release_conn_addrinfo is for.
> 3. Leave nconnhost and naddr, non-zero.
I agree that it would be good to do that pqReleaseConnHost and
release_conn_addrinfo. But also here, I don't see any issues caused by
not doing that currently.
So overall I agree pqReleaseConnHost and release_conn_addrinfo can be
improved for easier safe usage in the future, but I don't think those
improvements should be grouped into the same commit with an actual
bugfix.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix calloc check if oom (PQcancelCreate)
2024-05-27 16:47 Re: Fix calloc check if oom (PQcancelCreate) Jelte Fennema-Nio <[email protected]>
@ 2024-05-27 17:28 ` Ranier Vilela <[email protected]>
1 sibling, 0 replies; 7+ messages in thread
From: Ranier Vilela @ 2024-05-27 17:28 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers
Em seg., 27 de mai. de 2024 às 13:47, Jelte Fennema-Nio <[email protected]>
escreveu:
> On Mon, 27 May 2024 at 18:16, Ranier Vilela <[email protected]> wrote:
> > Is it mandatory to call PQcancelFinish in case PQcancelCreate fails?
>
>
> Yes, see the following line in the docs:
>
> Note that when PQcancelCreate returns a non-null pointer, you must
> call PQcancelFinish when you are finished with it, in order to dispose
> of the structure and any associated memory blocks. This must be done
> even if the cancel request failed or was abandoned.
>
> Source:
> https://www.postgresql.org/docs/17/libpq-cancel.html#LIBPQ-PQCANCELCREATE
>
> > IMO, I would expect problems with users.
>
> This pattern is taken from regular connection creation and is really
> common in libpq code so I don't expect issues. And even if there were
> an issue, your v1 patch would not be nearly enough. Because you're
> only freeing the connhost and addr field now in case of OOM. But there
> are many more fields that would need to be freed.
>
> >> And that function will free any
> >> partially initialized PGconn correctly. This is also how
> >> pqConnectOptions2 works.
> >
> > Well, I think that function *pqReleaseConnHost*, is incomplete.
> > 1. Don't free connhost field;
>
> ehm... it does free that afaict? It only doesn't set it to NULL. Which
> indeed would be good to do, but not doing so doesn't cause any issues
> with it's current 2 usages afaict.
>
> > 2. Don't free addr field;
>
> That's what release_conn_addrinfo is for.
>
> > 3. Leave nconnhost and naddr, non-zero.
>
> I agree that it would be good to do that pqReleaseConnHost and
> release_conn_addrinfo. But also here, I don't see any issues caused by
> not doing that currently.
>
> So overall I agree pqReleaseConnHost and release_conn_addrinfo can be
> improved for easier safe usage in the future, but I don't think those
> improvements should be grouped into the same commit with an actual
> bugfix.
>
Thanks for detailed comments.
I agreed to apply the trivial fix.
Would you like to take charge of pqReleaseConnHost and
release_conn_addrinfo improvements?
best regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix calloc check if oom (PQcancelCreate)
2024-05-27 16:47 Re: Fix calloc check if oom (PQcancelCreate) Jelte Fennema-Nio <[email protected]>
@ 2024-05-27 17:42 ` Daniel Gustafsson <[email protected]>
2024-05-27 17:51 ` Re: Fix calloc check if oom (PQcancelCreate) Ranier Vilela <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Daniel Gustafsson @ 2024-05-27 17:42 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: Ranier Vilela <[email protected]>; pgsql-hackers
> On 27 May 2024, at 18:47, Jelte Fennema-Nio <[email protected]> wrote:
> So overall I agree pqReleaseConnHost and release_conn_addrinfo can be
> improved for easier safe usage in the future, but I don't think those
> improvements should be grouped into the same commit with an actual
> bugfix.
I have pushed the fix for the calloc check for now.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix calloc check if oom (PQcancelCreate)
2024-05-27 16:47 Re: Fix calloc check if oom (PQcancelCreate) Jelte Fennema-Nio <[email protected]>
2024-05-27 17:42 ` Re: Fix calloc check if oom (PQcancelCreate) Daniel Gustafsson <[email protected]>
@ 2024-05-27 17:51 ` Ranier Vilela <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Ranier Vilela @ 2024-05-27 17:51 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; pgsql-hackers
Em seg., 27 de mai. de 2024 às 14:42, Daniel Gustafsson <[email protected]>
escreveu:
> > On 27 May 2024, at 18:47, Jelte Fennema-Nio <[email protected]> wrote:
>
> > So overall I agree pqReleaseConnHost and release_conn_addrinfo can be
> > improved for easier safe usage in the future, but I don't think those
> > improvements should be grouped into the same commit with an actual
> > bugfix.
>
> I have pushed the fix for the calloc check for now.
>
Thanks Daniel.
best regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-05-27 17:51 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-07-26 08:21 pg_walfile_name_offset can return inconsistent values Kyotaro Horiguchi <[email protected]>
2019-07-26 09:30 ` Jehan-Guillaume de Rorthais <[email protected]>
2022-12-15 01:04 [PATCH 07/16] +nondecimal Justin Pryzby <[email protected]>
2024-05-27 16:47 Re: Fix calloc check if oom (PQcancelCreate) Jelte Fennema-Nio <[email protected]>
2024-05-27 17:28 ` Re: Fix calloc check if oom (PQcancelCreate) Ranier Vilela <[email protected]>
2024-05-27 17:42 ` Re: Fix calloc check if oom (PQcancelCreate) Daniel Gustafsson <[email protected]>
2024-05-27 17:51 ` Re: Fix calloc check if oom (PQcancelCreate) Ranier Vilela <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox