Re: A few message wording/formatting cleanup patches

From: Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: A few message wording/formatting cleanup patches
Date: 2026-06-02 06:28:07
Message-ID: CAExHW5uDMr4x9=is8MwKzEzv3YVVTv_q36XtadMp=b5vEyJcRg@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Jun 2, 2026 at 10:13 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> Hello. Thank you for the comments.
>
> At Mon, 1 Jun 2026 17:18:13 +0530, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote in
> > 1. There are other places in the function where we use similar code.
> > Those places call initStringInfo just before getRelationDescription()
> > is called and then pfree() StringInfoData.data after it is added to
> > the object description.
>
> That's a good point. I hadn't paid much attention to that because the
> allocation is short-lived, but matching the surrounding code is
> probably better. I've added pfree() in the attached patch.
>
> > 2. Why do we want to capture the output of getObjectDescription() in a
> > StringInfo and then add it to the buffer? I think we can pass
> > getObjectDescription directly as an argument to appendStringInfo()
> > similar to the code for case AttrDefaultRelationId.
>
> I considered returning strings from helper functions such as
> getRelationDescription(), but that would affect a number of similar
> helper functions and broaden the scope of the patch.
>
> While working on this, I noticed that the string returned by
> getObjectDescription() in the AttrDefaultRelationId case is not
> pfree'd. Since that appears unrelated to this patch, I left it as-is.
>
> I also dropped the assertions on the temporary description strings.
> They were mainly there while developing the patch and don't seem
> necessary in the final version.

Thanks.

I like the idea of pfree'ing the string returned by
getObjectDescriptor() since we also pffree other descriptor strings.
But just doing it for these changes doesn't look consistent.

Attached patchset with some more changes to bring the new code inline
with the surrounding code. The changes are
1. rename objdesc to rel. The latter is used in the surrounding code.
2. getObjectDescriptor() is invoked directly from appendStringInfo.
3. Place InitStringInfo() closer to the appendStringInfo call like
surrounding code

None of these changes are compulsory. If you think any of the changes
are acceptable to you, please provide a single patch absorbing them
into your patch. Otherwise, let the committer decide.

0008 is your patch as is
0009 is my cosmetic changes

The numbering starts from 0008 since I create all SQL/PGQ patches for
various fixes from the same branch. Attached patches are applicable
independently.

--
Best Wishes,
Ashutosh Bapat

Attachment Content-Type Size
v20260602-0009-Cosmetic-adjustments.patch text/x-patch 5.7 KB
v20260602-0008-Make-propgraph-object-descriptions-transla.patch text/x-patch 6.2 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2026-06-02 06:30:40 Re: Improve pg_stat_statements scalability
Previous Message Andrei Lepikhov 2026-06-02 06:25:46 Re: hashjoins vs. Bloom filters (yet again)