| 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 |
| 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) |