Thread: Forbid to DROP temp tables of other sessions

Daniil Davydov <3danissimo@gmail.com> writes:
> I noticed that TRUNCATE and ALTER commands on temporary tables of other
> sessions produce an error "cannot truncate/alter temporary tables of other
> sessions". But are there any reasons to allow us to DROP such tables?
> It seems to me that the only case when we may need it is the removal of
> orphan tables. But the autovacuum is responsible for this and it uses a
> different functionality. I'm wondering if there are any other cases. If
> not, can we just handle it for example in ExecDropStmt and produce an error
> like "cannot drop temporary tables of other sessions"?

If autovacuum can do it, I don't see a reason to prevent superusers
from doing it manually.

            regards, tom lane

> On 14 Nov 2024, at 11:55, Daniil Davydov <3danissimo@gmail.com> wrote:
>
> On Wed, Oct 30, 2024 at 7:32 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
>
>> Good catch. I agree with this being an unwarranted behaviour.
>> A minor comment from my end is the wording of the error message.
>> Based on the Postgresql error message style huide, something like this could be better,
>> "could not access temporary relations of other sessions".
>> --
>> Regards,
>> Rafia Sabih
>> CYBERTEC PostgreSQL International GmbH
>>
> Thanks for your comment. I attach a patch with a fixed error message.
> Also you can find it in commit fest
> (https://commitfest.postgresql.org/51/5379/)

I suspect that protection of temp tables was broken by 00d1e02be249. And I'd suggest fixing it in a line with how it
workedbefore. Changes to locking mechanism is kind of a super subtle matters, it is really hard to bring this checks
herewithout breaking something else. Maybe not immidiately. but still. I'd suggest fixing somewhere around
RelationAddBlocks().But be sure to check all code pathes that lead to this check. 

Also, having an isolation test is nice. But do we actually do isolation tests with PL\pgSQL?

Thanks!


Best regards, Andrey Borodin.

Hi,

On 2024-11-21 23:52:52 +0300, Andrey M. Borodin wrote:
> I suspect that protection of temp tables was broken by 00d1e02be249.

I think we might have broken this in multiple ways in recent releases. In 17
one can even read the data from the other relation when using a sequential
scan, because that'll go through a streaming read and from there directly to
StartReadBuffers(), bypassing the check in ReadBufferExtended().


> And I'd suggest fixing it in a line with how it worked before. Changes to
> locking mechanism is kind of a super subtle matters, it is really hard to
> bring this checks here without breaking something else. Maybe not
> immidiately. but still. I'd suggest fixing somewhere around
> RelationAddBlocks(). But be sure to check all code pathes that lead to this
> check.

Yea, I don't think the lock approach would work that well.  However, I don't
love having to put RELATION_IS_OTHER_TEMP() checks everywhere either. After
all we seem to have introduced two independent oversights related to this...

I wonder if we could handle this by having a few locations explicitly opt-in
to accessing another database's temp table and erroring out everywhere else -
there's not that many places we need to do so.


> Also, having an isolation test is nice. But do we actually do isolation
> tests with PL\pgSQL?

There are several other tests creating functions. But I think this one goes a
bit too far...


Greetings,

Andres Freund

> On 22 Nov 2024, at 03:02, Andres Freund <andres@anarazel.de> wrote:
>
> I don't
> love having to put RELATION_IS_OTHER_TEMP() checks everywhere either.

+1. I do not understand why this restriction (protecting temp tables from access) is a responsibility of the buffer
manager.

Actually, I like the idea that superuser knows better what to do.
What if we say it's not a bug, but a feature. Will it break some contracts with user or some functionality?

It seems that protection of temp tables should belong to ACL stuff. And in a logic of this subsystem would be natural
tojust allow superuser do whatever they want with. 

Is it some lunatic idea? Or does it make some sense?


Best regards, Andrey Borodin.

Hi,
I'm sorry for the long lull.
Considering that it is still important for some users to be able to
clean other sessions' temporary directories, I suggest adding a GUC
that will allow superuser to do this.
We keep only one option - to drop tables, because only this feature
works properly in postgres by now.

The ability to read and modify other session's temp tables is broken
(I mean INSERT, UPDATE, DELETE, SELECT queries) and needs to be
disabled.
I wrote about broken INSERTs here :
https://www.postgresql.org/message-id/CAJDiXgj6TBzn%3D6Ezx7%2B9BNa9HpBitBU%2BMuv-N3mHeN_Zs3NBDw%40mail.gmail.com

What do you think about this idea?

--
Best regards,
Daniil Davydov

On Thu, 14 Nov 2024 at 14:26, Daniil Davydov <3danissimo@gmail.com> wrote:
>
> On Wed, Oct 30, 2024 at 7:32 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
>
> > Good catch. I agree with this being an unwarranted behaviour.
> > A minor comment from my end is the wording of the error message.
> > Based on the Postgresql error message style huide, something like this could be better,
> > "could not access temporary relations of other sessions".
> > --
> > Regards,
> > Rafia Sabih
> > CYBERTEC PostgreSQL International GmbH
> >
> Thanks for your comment. I attach a patch with a fixed error message.
> Also you can find it in commit fest
> (https://commitfest.postgresql.org/51/5379/)

I noticed that the following Andrey's comment regarding the isolation
test from [1] and Andres's comment  from [2] are pending. I'm changing
the commitfest entry to Waiting on Author, please provide an updated
patch and update it to Needs review.
[1] - https://www.postgresql.org/message-id/9A1C6763-A293-4EDD-A462-EFCF93BA2909%40yandex-team.ru
[2] - https://www.postgresql.org/message-id/4xxau766dofbwugeyvjftra3g5f7ifaal2clgrbpr7jqotr4av%40d3ige2krpoza

Regards,
Vignesh

Hi,

On Sun, Mar 16, 2025 at 7:53 PM vignesh C <vignesh21@gmail.com> wrote:
> I noticed that the following Andrey's comment regarding the isolation
> test from [1] and Andres's comment  from [2] are pending. I'm changing
> the commitfest entry to Waiting on Author, please provide an updated
> patch and update it to Needs review.

Thanks for reading it.
I saw [2] and introduced a possible solution in my last letter. In
short : we can have a GUC variable that will permit superuser to drop
temp tables of other sessions. Thus, we have a single location in code
that will check whether we can perform operations with other temp
tables. As far as I understand, this is exactly what Andres wrote
about.
Also, it is difficult for me to express my opinion on [1] at the
moment. I can say for sure that the tests will change when we agree on
the behavior of the code. Therefore, I suggest postponing the
resolution of this issue.

> I suggest adding a GUC that will allow superuser to do this
Waiting for your feedback on this issue :)

--
Best regards,
Daniil Davydov

Hi,

On Sun, Mar 16, 2025 at 7:53 PM vignesh C <vignesh21@gmail.com> wrote:
> I noticed that the following Andrey's comment regarding the isolation
> test from [1] and Andres's comment  from [2] are pending. I'm changing
> the commitfest entry to Waiting on Author, please provide an updated
> patch and update it to Needs review.

Thanks for reading it.
I saw [2] and introduced a possible solution in my last letter. In
short : we can have a GUC variable that will permit superuser to drop
temp tables of other sessions. Thus, we have a single location in code
that will check whether we can perform operations with other temp
tables. As far as I understand, this is exactly what Andres wrote
about.

It seems that protection of temp tables should belong to ACL stuff. And in a logic of this subsystem would be natural to just allow superuser do whatever they want with.

Hi,

On Mon, Mar 17, 2025 at 1:15 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> It’s seems like the bug “session A can read and write to session B’s tables” has gotten lost in this thread that
pertainsto something related but different. .... Solving the bug is not going to involve adding a new GUC. 
I don't think it's worth putting this in a separate discussion. Now
everything is moving towards the fact that the superuser will be
prohibited from changing the temporary tables of other sessions. In
fact, he can't do it anyway (except for DROP command) - see [1]. But
now the error for INSERT, UPDATE, DELETE and SELECT commands may not
surface immediately due to errors in the code. The only question now
is whether superuser should be allowed to DROP these other temp
tables. Since opinions are divided, I suggested adding a GUC that will
only control this feature.

> If they specify the precise pg_temp schema to affect they likely didn’t make a mistake.
Yep, I agree. However, the features of the postgres kernel do not
allow the superuser to correctly perform INSERT, UPDATE, DELETE,
SELECT operations, because temporary table's pages cannot be stored in
shared buffers.

> If instead we are discussing the owner of the temporary table there is probably a discussion to be had and decision
tobe documented somewhere - maybe that central place of testing being wished for. 
As far as I understand, only superuser and table's owner (within
session) can access the temp tables of session. We want CRUD
operations to be performed only by the owner.

--
Best regards,
Daniil Davydov

[1] https://www.postgresql.org/message-id/CAJDiXgj6TBzn=6Ezx7+9BNa9HpBitBU+Muv-N3mHeN_Zs3NBDw@mail.gmail.com

Hi,

On Mon, Mar 17, 2025 at 1:52 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> My understanding is the limitation of an owner of a temporary relation in one session being disallowed to alter its
contentsfrom another session is an implementation consequence, and not some fundamental model restriction. 
I would say this: working with temporary tables (in the postgres
kernel) is so very different from working with regular heap tables
that such a limitation can be considered fundamental. We simply will
not be able to organize coordinated work on the temporary table from
different OS processes (for INSERT, UPDATE, DELETE).

> Minimally informed thinking, associate the specific pg_temp namespace with a procid.  Where this limitation exists,
whichseems like middle management, compare the proc of the namespace to the executor.  Pass the role and also an enum
ofaction type (CRUD, drop, truncate, lock, etc…).  If the procs match all good.  Superuser cannot bypass CRUD and
similaras that is the limitation being implemented here.  And the owner cannot bypass anything (exceptions could be
addedas desired). 
>
> Centralizing things a bit though…maybe something like the relcache (for namespaces…) so you cannot even get a handle
onthe namespace if you don’t supply the info and pass the checks.  Don’t really know enough to say where/how to
implement“if you forget to call this check all commands that can reference tables will fail”. 
I'm sorry, I probably don't quite understand what this is about, so
I'll just describe how it works now. If a superuser wants to access
other temp table, he must specify schema_name in request (for example
: INSERT INTO pg_temp_N.test .....). N is the id of the owner process.
Thus, postgres will call RangeVarGetRelidExtended to find the given
relation's oid. It is at this point that we can step in and check
whether the caller can work with the specified schema. It is
elementary to understand that the schema does belong to another
session. Right now, there is a bug in the code that mistakenly
recognizes the table in such a case as permanent (not temporary), so
we cannot do what I just described. So, we have to get rid of this bug
and decide whether we reserve the right for the superuser to DROP such
tables.

I hope this remark will be useful.

--
Best regards,
Daniil Davydov

Hi,
I see that the presence of isolation tests in the patch is
controversial. First things first, let's concentrate on fixing the
bug.
I attach a new version of patch (for `master` branch) to this letter.
It contains better comments and a few small improvements.

P.S.
Sorry for bad formatting in previous letter (idk how to fix it in gmail client)

--
Best regards,
Daniil Davydov

Hi,

I have some comments:

1. namespace.c, if relation->schemaname is pg_temp but myTempNamespace 
isn't set, the error information might be misleading. Consider checking 
OidIsValid(myTempNamespace) first.

2."you have not any temporary relations" --> "you have no any temporary 
relations"

3. Regarding to the code "strncmp(nspname, "pg_temp", 7)", is it ok when 
the nspname contains something like "pg_temp_1234"? I think we should 
use strcmp instead of strncmp for exact matching.

Thanks,
Steven

在 2025/3/17 17:03, Daniil Davydov 写道:
> Hi,
> I see that the presence of isolation tests in the patch is
> controversial. First things first, let's concentrate on fixing the
> bug.
> I attach a new version of patch (for `master` branch) to this letter.
> It contains better comments and a few small improvements.
> 
> P.S.
> Sorry for bad formatting in previous letter (idk how to fix it in gmail client)
> 
> --
> Best regards,
> Daniil Davydov

Hi,

On Mon, Mar 17, 2025 at 4:48 PM Steven Niu <niushiji@gmail.com> wrote:
>
> 1. namespace.c, if relation->schemaname is pg_temp but myTempNamespace
> isn't set, the error information might be misleading. Consider checking
> OidIsValid(myTempNamespace) first.
Could you please clarify exactly which place in the code we are
talking about? I think we handle this case in the
LookupExplicitNamespace call (with all appropriate error information).

>
> 2."you have not any temporary relations" --> "you have no any temporary
> relations"
I am not an English speaker, but it seems that "have not" would be
more correct. Someone has to judge us :)

>
> 3. Regarding to the code "strncmp(nspname, "pg_temp", 7)", is it ok when
> the nspname contains something like "pg_temp_1234"? I think we should
> use strcmp instead of strncmp for exact matching.
Great catch! I'll fix it. Please, see v3 patch.

--
Best regards,
Daniil Davydov

在 2025/3/17 18:13, Daniil Davydov 写道:
> Hi,
> 
> On Mon, Mar 17, 2025 at 4:48 PM Steven Niu <niushiji@gmail.com> wrote:
>>
>> 1. namespace.c, if relation->schemaname is pg_temp but myTempNamespace
>> isn't set, the error information might be misleading. Consider checking
>> OidIsValid(myTempNamespace) first.
> Could you please clarify exactly which place in the code we are
> talking about? I think we handle this case in the
> LookupExplicitNamespace call (with all appropriate error information).
> 

I mean RangeVarGetRelidExtended(), you deleted the following code:

if (!OidIsValid(myTempNamespace))
            relId = InvalidOid; /* this probably can't happen? */


>>
>> 2."you have not any temporary relations" --> "you have no any temporary
>> relations"
> I am not an English speaker, but it seems that "have not" would be
> more correct. Someone has to judge us :)
> 
>>
>> 3. Regarding to the code "strncmp(nspname, "pg_temp", 7)", is it ok when
>> the nspname contains something like "pg_temp_1234"? I think we should
>> use strcmp instead of strncmp for exact matching.
> Great catch! I'll fix it. Please, see v3 patch.
> 
> --
> Best regards,
> Daniil Davydov

Hi,

On Mon, Mar 17, 2025 at 5:33 PM Steven Niu <niushiji@gmail.com> wrote:
>
> I mean RangeVarGetRelidExtended(), you deleted the following code:
>
> if (!OidIsValid(myTempNamespace))
>             relId = InvalidOid; /* this probably can't happen? */

Hm, I got it. Let's start with the fact that the comment "this
probably can't happen?" is incorrect. Even if we don't have a
temporary namespace in our session, we still can specify "pg_temp_N"
in the psql query.
Next, if relation->schemaname is pg_temp, then we firstly call
LookupExplicitNamespace, where you can find if
(!OidIsValid(myTempNamespace)) check.

--
Best regards,
Daniil Davydov

在 2025/3/17 18:56, Daniil Davydov 写道:
> Hi,
> 
> On Mon, Mar 17, 2025 at 5:33 PM Steven Niu <niushiji@gmail.com> wrote:
>>
>> I mean RangeVarGetRelidExtended(), you deleted the following code:
>>
>> if (!OidIsValid(myTempNamespace))
>>              relId = InvalidOid; /* this probably can't happen? */
> 
> Hm, I got it. Let's start with the fact that the comment "this
> probably can't happen?" is incorrect. Even if we don't have a
> temporary namespace in our session, we still can specify "pg_temp_N"
> in the psql query.
> Next, if relation->schemaname is pg_temp, then we firstly call
> LookupExplicitNamespace, where you can find if
> (!OidIsValid(myTempNamespace)) check.
> 
> --
> Best regards,
> Daniil Davydov

If the (relation->relpersistence == RELPERSISTENCE_TEMP) can ensure the 
myTempNamespace is always valid, then my comment can be ignored. 
Otherwise I think the modified RangeVarGetRelidExtended() should keep 
check of myTempNamespace, like this:

if (relation->relpersistence == RELPERSISTENCE_TEMP)
{
   Oid namespaceId;

  if (!OidIsValid(myTempNamespace))
      relId = InvalidOid; /* this probably can't happen? */
  else
  {
    if (relation->schemaname)
    {
      namespaceId = LookupExplicitNamespace(relation->schemaname, 
missing_ok);
      if (namespaceId != myTempNamespace)
      {
          ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                   errmsg("could not access temporary relations of other 
sessions")));
      }
   }
   else
   {
      namespaceId = myTempNamespace;
      Assert(OidIsValid(namespaceId));
   }
   if (missing_ok && !OidIsValid(namespaceId))
      relId = InvalidOid;
   else
      relId = get_relname_relid(relation->relname, namespaceId);
  }
...
...

Thanks,
Steven

>
> 2."you have not any temporary relations" --> "you have no any temporary
> relations"
I am not an English speaker, but it seems that "have not" would be
more correct. Someone has to judge us :)

Hi,

On Mon, Mar 17, 2025 at 8:29 PM Steven Niu <niushiji@gmail.com> wrote:
>
> If the (relation->relpersistence == RELPERSISTENCE_TEMP) can ensure the
> myTempNamespace is always valid, then my comment can be ignored.

No, even if we see a temporary table in RangeVarGetRelidExtended,
MyTempNamespace still can be `InvalidOid` (I mentioned it in the
previous letter).
Thus, comment like "this probably can't happen?" should be removed.

> Otherwise I think the modified RangeVarGetRelidExtended() should keep
> check of myTempNamespace, like this:
>
> if (relation->relpersistence == RELPERSISTENCE_TEMP)
> {
>    Oid namespaceId;
>
>   if (!OidIsValid(myTempNamespace))
>       relId = InvalidOid; /* this probably can't happen? */
>  ...

OK, let's assume that MyTempNamespace == InvalidOid and caller
specified "pg_temp_N" in his query. In this case we want to throw an
error, because access to the other temp tables is prohibited.
If we keep code like "if (!OidIsValid(myTempNamespace)) => relId =
InvalidOid", eventually the caller will get an error "relation ...
does not exist".
Yes, we have saved the caller from making mistakes, but we are giving
the wrong error message.

In my realization the caller will get the correct error message, and I
still think that we should keep v3 patch logic.

--
Best regards,
Daniil Davydov

Hi,

On Mon, Mar 17, 2025 at 10:09 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Monday, March 17, 2025, Daniil Davydov <3danissimo@gmail.com> wrote:
>>
>>
>> >
>> > 2."you have not any temporary relations" --> "you have no any temporary
>> > relations"
>> I am not an English speaker, but it seems that "have not" would be
>> more correct. Someone has to judge us :)
>>
>
> Both are not good.
>
> “pg_temp was specified but it contains no relations” [1]

That sounds reasonable. I'll fix it. Thanks!

> But why are we promoting this situation to an error?  It should be a relation not found error just like any other and
notits own special case. 
> The fact we create pg_temp only as it is needed is an implementation detail that should not be visible to the user.

1)
Error message has no mention of a non-existent schema. "Schema has no
relations" => "Given relation not found in empty schema". It seems to
me that these are equivalent statements.

2)
Is this really the implementation detail that we want to hide from the
user? User can just run "select pg_my_temp_schema();" and see that
there is no temp schema in the current session.
Don't get me wrong - I can agree with that, but for now it seems odd to me...
Steven Niu also mentioned this issue, but IMO we must give the most
accurate description of the problem - tell "relation not found" only
if we have temp namespace, but not specified relation in it.

Please see v4 patch (only comment change).

--
Best regards,
Daniil Davydov

Hi,

On Tue, Mar 18, 2025 at 2:36 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> "I want to give a better error message" is not a good enough reason to change this long-standing behavior in a
back-patchablebug fix. 
>.... and I do not agree that it is an improvement worth making in HEAD.

OK, In this case I'd rather agree.


On Tue, Mar 18, 2025 at 3:30 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> I sound like a broken record but this is a bug fix; introducing a GUC and changing unrelated behaviors is not
acceptablein a back-patch. 
>
> Superusers have been able to drop the temporary tables of other sessions for a long time - now is not the place to
changethat behavior. 

Well, we can keep this option for the superuser, but we need to
explicitly indicate that we are inside the DROP operation.
Please see v5 patch. It is a bit experimental :
1) The original behavior in the LookupExplicitNamespace function has
been returned.
2) New RVROption has been added to indicate that we are inside the
DROP operation. Thus, superuser can drop other temp tables.

>
> Separately:
>
> Is the general logic here that we assume r->relpersistence is RELPERSISTENCE_PERMANENT until and unless we can prove
itto be RELPERSISTENCE_TEMP? 
>
> I'm trying to figure out whether there is still an issue when dealing with an unqualified name that would be found in
thetemporary schema. 
> We've obviously already marked it permanent since we didn't have pg_temp in the query to inform us otherwise.
> But if we are changing permanent to temporary later because of search_path resolution then why did we have to get it
rightin the case where pg_temp is specified? 
> I question whether "persistence" is something that gram.y should be dealing with at all.  Shouldn't that property be
solelydetermined in post-parse analysis via catalog lookup? 

Hm, let's dive into this question. Why do I consider the original
behavior to be incorrect?

1)
If schema is not specified, then gram.y marks the table as PERMANENT.
1.1)
If we are looking for our temporary table, then "pg_temp_N" is present
in search_path, and RelnameGetRelid will return us this table.
1.2)
If we are looking for other temporary table, then RelnameGetRelid will
return `InvalidOid` and we will get a "relation not found" error.

2)
If schema is specified, then gram.y ALSO marks the table as PERMANENT.
2.1)
If we are looking for our temporary table, then
LookupExplicitNamespace will return us MyTempNamespace and we can get
our table without search_path lookup.
2.2)
If we are looking for other temporary table, then
LookupExplicitNamespace will return some valid oid, and we can get
other temp table without search_path lookup. Then, we will perform any
specified operation, assuming that we are working with a PERMANENT
table. This is a bug.

Let's summarize. If schema is not specified, we can safely mark the
table as PERMANENT, because we always can get the table from
search_path (and if the schema is not specified, then it is obvious
that the user wants to refer specifically to his table).
BUT, if schema is specified, we must know that the given relation is
TEMP before calling RangeVarGetRelidExtended (otherwise, there will be
an error, which I wrote about in paragraph 2.2).

>
> IOW, the original code here seems incorrect if this is the definitive place to determine relpersistence.  in "case
1:",where there is no schema name, one cannot deduce relpersistence and it should remain NULL, IMO (not knowing what
thatmight break...).  The code this patch replaces is wrong for the same reason. 

I hope that I managed to clarify this issue. As far as "pg_temp_"
prefix is reserved by the postgres kernel, we can definitely say that
we have encountered a temporary table when we see this prefix. IMO
there is no problem, that gram.y will do it.

>
> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index 271ae26cbaf..f68948d475c 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -19424,7 +19424,11 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner)
>   break;
>   }
>
> - r->relpersistence = RELPERSISTENCE_PERMANENT;
> + if (r->schemaname && strncmp(r->schemaname, "pg_temp", 7) == 0)
> + r->relpersistence = RELPERSISTENCE_TEMP;
> + else
> + r->relpersistence = RELPERSISTENCE_PERMANENT;
> +
>   r->location = position;
>
> The qualified name variant is fine since r->schemaname must be present by definition.  Passing through the same if
block,once with schemaname null (in makeRangeVar) and once with it populated (end of makeRangeVarFromQualifiedName) is
abit annoying. 
>
> makeRangeVar has the same problem, assuming permanent when the schemaname argument is null.

Thank you for noticing it. I suggest we first confirm that the logic
(with persistence definition) remains in gram.y and then fix this
problem.

--
Best regards,
Daniil Davydov

Hi,

On Wed, Mar 19, 2025 at 6:32 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> I'm probably done trying to help with this one - it's beyond my ability and desire to contribute to meaningfully.  It
seemsto need to be escalated if the regression has a chance of being understood and fixed. 
> Some good candidates for helping out are copied already so I'm just hoping one or more of them chimes in.  I'll
repeatmy suggestion to start a new thread and let this one die.  The answer to $subject is no. 

Well, the name of the thread has stopped reflecting what is being
discussed here. I may start a new discussion in the near future, but
as part of this discussion I would like to make sure that people agree
with the statements from my previous letter.

Thanks for your help.

--
Best regards,
Daniil Davydov

Hi, hackers,

During browsing the code, I found one missing PQclear in function 
StreamLogicalLog(). It's a very small problem as it only happens in 
error condition. However since another similar patch was accepted,
maybe we should also fix this one.


https://www.postgresql.org/message-id/flat/3DA7CECD-5A05-416D-8527-ABD794AEFE8B%40yesql.se#c5d662ba7bdb07e56ddbd9aaa90dea5d



Regards,
Steven

> On 19 Mar 2025, at 06:38, Steven Niu <niushiji@gmail.com> wrote:

> During browsing the code, I found one missing PQclear in function StreamLogicalLog(). It's a very small problem as it
onlyhappens in error condition. However since another similar patch was accepted, 
> maybe we should also fix this one.

Thanks for the report, the patch looks reasonable so I'll have a look at
applying it once back from travelling.

--
Daniel Gustafsson

> On 19 Mar 2025, at 11:03, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 19 Mar 2025, at 06:38, Steven Niu <niushiji@gmail.com> wrote:
>
>> During browsing the code, I found one missing PQclear in function StreamLogicalLog(). It's a very small problem as
itonly happens in error condition. However since another similar patch was accepted, 
>> maybe we should also fix this one.
>
> Thanks for the report, the patch looks reasonable so I'll have a look at
> applying it once back from travelling.

Applied to head, thanks!

--
Daniel Gustafsson