| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Corey Huinker <corey(dot)huinker(at)gmail(dot)com> |
| Cc: | Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: Import Statistics in postgres_fdw before resorting to sampling. |
| Date: | 2025-11-24 09:25:13 |
| Message-ID: | F9C73EF2-F977-46E4-9F61-B6CF72BF1A69@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Nov 23, 2025, at 15:27, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>
> My apologies for the delayed response.
>
> Valuable responses are worth waiting for.
>
> I've reorganized some things a bit, mostly to make resource cleanup simpler.
> <v4-0001-Add-remote-statistics-fetching-to-postgres_fdw.patch>
Few comments:
1 - commit message
```
effort and the user is better of setting fetch_stats to false for that
```
I guess “of” should be “off”
2 - postgres-fdw.sgml
```
+ server, determines wheter an <command>ANALYZE</command> on a foreign
```
Typo: wheter => whether
3 - postgres-fdw.sgml
```
+ data sampling if no statistics were availble. This option is only
```
Typo: availble => available
4 - option.c
```
+ /* fetch_size is available on both server and table */
+ {"fetch_stats", ForeignServerRelationId, false},
+ {"fetch_stats", ForeignTableRelationId, false},
```
In the comment, I guess fetch_size should be fetch_stats.
5 - analyze.c
```
+ * XXX: Should this be it's own fetch type? If not, then there might be
```
Typo: “it’s own” => “its own”
6 - analyze.c
```
+ case FDW_IMPORT_STATS_NOTFOUND:
+ ereport(INFO,
+ errmsg("Found no remote statistics for \"%s\"",
+ RelationGetRelationName(onerel)));
```
`Found no remote statistics for \"%s\””` could be rephrased as `""No remote statistics found for foreign table \"%s\””`, sounds better wording in server log.
Also, I wonder if this message at INFO level is too noisy?
7 - postgres_fdw.c
```
+ default:
+ ereport(INFO,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("Remote table %s.%s does not support statistics.",
+ quote_identifier(remote_schemaname),
+ quote_identifier(remote_relname)),
+ errdetail("Remote relation if of relkind \"%c\"", relkind));
```
I think “if of” should be “is of”.
8 - postgres_fdw.c
```
+ errmsg("Attribute statistics found for %s.%s but no columns matched",
+ quote_identifier(schemaname),
+ quote_identifier(relname))));
```
Instead of using "%s.%s” and calling quote_identifier() twice, there is a simple function to use: quote_qualified_identifier(schemaname, relname).
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Heikki Linnakangas | 2025-11-24 09:27:51 | Re: [PATCH] Avoid pallocs in async.c's SignalBackends critical section |
| Previous Message | Kirill Reshke | 2025-11-24 09:22:56 | Re: Add comments about fire_triggers argument in ri_triggers.c |