| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Álvaro Herrera <alvherre(at)kurilemu(dot)de> |
| Cc: | Pg Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: splitting pg_resetwal output strings |
| Date: | 2026-02-02 03:58:10 |
| Message-ID: | A2A10B88-A7B1-4757-A64F-94E769C91F28@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Jan 31, 2026, at 23:08, Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
>
> On 2026-Jan-31, Álvaro Herrera wrote:
>
>> This is not complete. It modifies PrintControlValues(), which is easy
>> because I just print each item in the entries.h file in the same order
>> they appear there. But for PrintNewControlValues() I'll need to add a
>> symbolic identifier to each string, that the code can use to scan the
>> array and print just those elements. I'll do that if there are no
>> objections to this idea here.
>
> It looks more or less like this. Patch 0001 is the same as before, and
> 0002 adds the symbolic names, which is used to create an enum and then
> to search for the correct lines to print in PrintNewControlValues.
>
> I decided to reuse simple_oid_list to store the integers values, which
> is kinda icky, so getting to something committable I think would have me
> add simple_int_list. Also, there's a few blocks that are duplicate
> cases of the same line measuring and printing logic; I suppose I should
> have a routine to simplify.
>
Hi Alvaro,
I don't think we necessarily need to add a simple_int_list. Since OIDs are not globally unique across all object types, we can reasonably treat ControlData items as "objects" and assign them OIDs within this context.
If we take that approach, the enum could be named ControlDataOid:
```
/* Define the string enums */
#define CONTROLDATA_LINE(symbol, description, fmt, ...) \
symbol,
enum {
#include "entries.h”
} ControlDataOid;
#undef CONTROLDATA_LINE
```
This makes reusing simple_oid_list feel appropriate. If you aren't fond of that idea, I’d suggest naming the enum ControlDataSymbol rather than ControlDataStrings, as the former better reflects what the members actually are.
Regarding patch 0002:
```
+#define CONTROLDATA_LINE(symbol, description, fmt, ...) \
+ if (simple_oid_list_member(&toprint, symbol)) \
+ { \
+ thislen = internal_wcswidth(_(description), \
+ strlen(_(description)), \
+ encoding); \
+ if (thislen > maxlen) \
+ maxlen = thislen; \
}
+#include "entries.h"
+#undef CONTROLDATA_LINE
```
The code block inside the first if statement is missing an indentation level.
Otherwise, the patch looks good to me. I've tested it and confirmed that calculating the padding at runtime generates better-looking output.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chengpeng Yan | 2026-02-02 04:05:36 | Re: [PATCH] ANALYZE: hash-accelerate MCV tracking for equality-only types |
| Previous Message | wenhui qiu | 2026-02-02 03:58:02 | Re: Non-committer reviews: is it helpful? |