Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vcEIS-003ML5-2j for pgsql-hackers@arkaria.postgresql.org; Sun, 04 Jan 2026 02:56:49 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vcEIR-00ENG0-2X for pgsql-hackers@arkaria.postgresql.org; Sun, 04 Jan 2026 02:56:48 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vcEIR-00ENFr-1C for pgsql-hackers@lists.postgresql.org; Sun, 04 Jan 2026 02:56:48 +0000 Received: from mail-oo1-xc35.google.com ([2607:f8b0:4864:20::c35]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vcEIP-0044MV-0M for pgsql-hackers@postgresql.org; Sun, 04 Jan 2026 02:56:47 +0000 Received: by mail-oo1-xc35.google.com with SMTP id 006d021491bc7-657509d761dso7179049eaf.0 for ; Sat, 03 Jan 2026 18:56:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1767495403; x=1768100203; darn=postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=/nO6oeFKMFBzpcvR8DbmOFZiSiPRm1xYurV4GM/LY/M=; b=k1pBfHfNklLG6R6+9FvKeNsIVYiKG6AOHE9urAHMj+5o1uZX5jZu073KqBTArznRet hm1MKGVFISfXKBIkkTkdZuuetPVt60SwAN5xzXT5YAIzwGI19xGNXVQzTglpZLw95Xko 4Vudmff/XP+iT3H/xYTWc8RiKFaLGp84LerUnXMO4FMg7dflRlF/hlDjVMqtLOL/pkpc wWSJmadwxXZeOCQuvPf3ZCAUKaUJYjalS86riIkvwRS5NWRBm6hJF0lizkZlYv0OnPOH S9x7ADb7vZvuscxIPMscFSvWQfIJLGz5eBF9G6xVUb+CX78y72my4OKmd7bOVHrQOzJ1 Vwbg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1767495403; x=1768100203; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=/nO6oeFKMFBzpcvR8DbmOFZiSiPRm1xYurV4GM/LY/M=; b=aB5vK5TCfBcZ8ICSIfKjxff6WRaqWrYbe+sbHDGGiblrZG+GLGHp9Ki9V+t4GYZ48e dAY+lCUPJIcVVDcP6basvNbgy+LKojfVzVntl74TVj48KiKvvaIIwnJg5nimICykvfIw p3WyCbjjduNlVfPUL500W7zHEUrYcF5OUJaMtcvmWpPKHl4twBu3RuY9mG257HF9KRzA QP2cc51jBXiHU9GMH7Zj9GIvkMnQyWiYRgPeS9eY+sOPqIc23d4v49e+8EAwh4d/OUsW JMhRZj3UTDHuvuclJgJKqWQdPA+Cxzii9g3H/U7orzTs/ZIXMAoBe8u7BZvH/2g4X7Cj 2AcQ== X-Forwarded-Encrypted: i=1; AJvYcCX3m/XQKAUln0lSvgevoIjzEiwixeDJIlVuSQCcjgXfYiZkjzYmoixTE6KHsM3Zuft/Jrt2aR5fgPBtJc2I@postgresql.org X-Gm-Message-State: AOJu0YwbZFSybGuBWa1XSxY7zDNXz9IWN9pTH8C8ZeQ1jwGEq5AYj3T6 /UWtwykSq3w6tDEXSm9p/Nsc6OrSg9vAd4T5LNrq/OMNaLxjGqQEa3RHSUCwu6rm2QCztjuX712 iJag7KWBNMKQTXOnHFg38QB5n7R71PZ0= X-Gm-Gg: AY/fxX5xUKQeG1l3znOgfu43MG0KD/Efk4HKDY44zwcMOCtT6o8JRpj2yS5ZLI/+6F2 H31vLV9heAbHFjt5WdLuTdOm/OPUi34K+i6yTlD48T7jBGFhtAA81hK1jJ9wvVLVkvtTPoBcIgO btgwAY0/obKTIkbBoLOdKfnszs+G2fR+7xGRRv2KFQXnbPspuW0fNedFv2X866RBipy5mbglhzi FidNC2tc+EPBLyiGvraiRwdKyGVVU+14otXSWdmruMmfphfk4oja29xNkmsbN6cQYfRYMq2TSAb xIjlem47zxyOSo91eqE046oTqPs= X-Google-Smtp-Source: AGHT+IHK8SqgEe26ebvByEnTUxBKTua9OEJLxdlHIkAjerNR4EMCkyaJz2g4Ct22AGaCnkyo9Ygu2Nf527sz3gsgGfI= X-Received: by 2002:a05:6820:626:b0:65c:fff1:bcf6 with SMTP id 006d021491bc7-65d0ea9d861mr18361742eaf.35.1767495403059; Sat, 03 Jan 2026 18:56:43 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Corey Huinker Date: Sat, 3 Jan 2026 21:56:31 -0500 X-Gm-Features: AQt7F2rM9ThEniHmIqDTx_9tLyXEbCakfQ_1w-Ho2UHNFEhGRM_uYacR-aSCAF0 Message-ID: Subject: Re: Import Statistics in postgres_fdw before resorting to sampling. To: Etsuro Fujita Cc: Michael Paquier , Ashutosh Bapat , pgsql-hackers@postgresql.org, jkatz@postgresql.org, nathandbossart@gmail.com Content-Type: multipart/alternative; boundary="000000000000a43f770647871787" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000a43f770647871787 Content-Type: text/plain; charset="UTF-8" > > > This version of the patch wouldn't fall back to the normal ANALYZE > processing anymore, so this documentation should be updated as such. > Also, as I think it's the user's responsibility to ensure the existing > statistics are up-to-date, as I said before, I think we should add a > note about that here. Also, as some users wouldn't be able to ensure > it, I'm wondering if the default should be false. > I agree, if there is no fallback, then the default should be false. When I was initially brainstorming this patch, Nathan Bossart had suggested making it the default because 1) that would be an automatic benefit to users and 2) the cost for attempting to import stats was small in comparison to a table stample, so it was worth the attempt. I still want users to get that automatic benefit, but if there is no fallback to sampling then the default only makes sense as false. > If the user wasn't able to ensure the statistics are up-to-date, I > think he/she might want to do remote ANALYZE *before* fetching the > statistics, not after trying to do so. So I think we could instead > provide this option as such. What do you think about that? Anyway, > I'd vote for leaving this for another patch, as I think it's a > nice-to-have option rather than a must-have one, as I said before. > I hadn't thought of that one. I think it has some merit, but I think we'd want the try-after case as well. So the settings would be: "off" (never remote analyze, default), "on" (always analyze before fetching), and "retry" (analyze if no stats were found). This feature feeds into the same thinking that the default setting did, which was to make this feature just do what is usually the smart thing, and do it automatically. Building it in pieces might be easier to get committed, but it takes away the dream of seamless automatic improvement, and establishes defaults that can't be changed in future versions. That dream was probably unrealistic, but I wanted to try. > > ISTM that the code is well organized overall. Here are a few comments: > Thanks! > > We don't use relallvisible and relallfrozen for foreign tables (note > that do_analyze_rel() calls vac_update_relstats() with relallvisible=0 > and relallfrozen=0 for them). Do we really need to retrieve (and > restore) them? > No, and as you stated, we wouldn't want to. The query was lifted verbatim from pg_dump, with a vague hope of moving the queries to a common library that both pg_dump and postgres_fdw could draw upon. But that no longer makes sense, so I'll fix. > +static const char *attstats_query_17 = > + "SELECT DISTINCT ON (s.attname) attname, s.null_frac, s.avg_width, " > + "s.n_distinct, s.most_common_vals, s.most_common_freqs, " > + "s.histogram_bounds, s.correlation, s.most_common_elems, " > + "s.most_common_elem_freqs, s.elem_count_histogram, " > + "s.range_length_histogram, s.range_empty_frac, > s.range_bounds_histogram " > + "FROM pg_catalog.pg_stats AS s " > + "WHERE s.schemaname = $1 AND s.tablename = $2 " > + "ORDER BY s.attname, s.inherited DESC"; > > I think we should retrieve the attribute statistics for only the > referenced columns of the remote table, not all the columns of it, to > reduce the data transfer and the cost of matching local/remote > attributes in import_fetched_statistics(). > I thought about this, and decided that we wanted to 1) avoid per-column round trips and 2) keep the remote queries simple. It's not such a big deal to add "AND s.attname = ANY($3)" and construct a '{att1,att2,"ATt3"}' string, as we already do in pg_dump in a few places. > > In fetch_remote_statistics() > > + if (server_version_num >= 180000) > + relation_sql = relstats_query_18; > + else if (server_version_num >= 140000) > + relation_sql = relstats_query_14; > + else > + relation_sql = relstats_query_default; > > I think that having the definition for each of relstats_query_18, > relstats_query_14 and relstats_query_default makes the code redundant > and the maintenance hard. To avoid that, how about building the > relation_sql query dynamically as done for the query to fetch all > table data from the remote server in postgresImportForeignSchema(). > Same for the attribute_sql query. > It may be a moot point. If we're not fetching relallfrozen, then the 14 & 18 cases are now the same, and since the pre-14 case concerns differentiating between analyzed and unanalyzed tables, we would just map that to 0 IF we kept those stats, but we almost never would because an unanalyzed remote table would not have the attribute stats necessary to qualify as a good remote fetch. So we're down to just one static query. > > That's all I have for now. I will continue to review the changes. > Much appreciated. --000000000000a43f770647871787 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

This version of the patch wouldn't fall back to the normal ANALYZE
processing anymore, so this documentation should be updated as such.
Also, as I think it's the user's responsibility to ensure the exist= ing
statistics are up-to-date, as I said before, I think we should add a
note about that here.=C2=A0 Also, as some users wouldn't be able to ens= ure
it, I'm wondering if the default should be false.
=
I agree, if there is no fallback, then the default should be= false. When I was initially brainstorming this patch, Nathan Bossart had s= uggested making it the default because 1) that would be an automatic benefi= t to users and 2) the cost for attempting to import stats was small in comp= arison to a table stample, so it was worth the attempt. I still want users = to get that automatic benefit, but if there is no fallback to sampling then= the default only makes sense as false.
=C2=A0
If the user wasn't able to ensure the statistics are up-to-date, I
think he/she might want to do remote ANALYZE *before* fetching the
statistics, not after trying to do so.=C2=A0 So I think we could instead provide this option as such.=C2=A0 What do you think about that?=C2=A0 Anyw= ay,
I'd vote for leaving this for another patch, as I think it's a
nice-to-have option rather than a must-have one, as I said before.

I hadn't thought of that one. I think it ha= s some merit, but I think we'd want the try-after case as well. So the = settings would be: "off" (never remote analyze, default), "o= n" (always analyze before fetching), and "retry" (analyze if= no stats were found). This feature feeds into the same thinking that the d= efault setting did, which was to make this feature just do what is usually = the smart thing, and do it automatically. Building it in pieces might be ea= sier to get committed, but it takes away the dream of seamless automatic im= provement, and establishes defaults that can't be changed in future ver= sions. That dream was probably unrealistic, but I wanted to try.
=
=C2=A0

ISTM that the code is well organized overall.=C2=A0 Here are a few comments= :

Thanks!
=C2=A0

We don't use relallvisible and relallfrozen for foreign tables (note that do_analyze_rel() calls vac_update_relstats() with relallvisible=3D0 and relallfrozen=3D0 for them).=C2=A0 Do we really need to retrieve (and restore) them?

No, and as you stated, w= e wouldn't want to. The query was lifted verbatim from pg_dump, with a = vague hope of moving the queries to a common library that both pg_dump and = postgres_fdw could draw upon. But that no longer makes sense, so I'll f= ix.
=C2=A0
+static const char *attstats_query_17 =3D
+=C2=A0 =C2=A0"SELECT DISTINCT ON (s.attname) attname, s.null_frac, s.= avg_width, "
+=C2=A0 =C2=A0"s.n_distinct, s.most_common_vals, s.most_common_freqs, = "
+=C2=A0 =C2=A0"s.histogram_bounds, s.correlation, s.most_common_elems,= "
+=C2=A0 =C2=A0"s.most_common_elem_freqs, s.elem_count_histogram, "= ;
+=C2=A0 =C2=A0"s.range_length_histogram, s.range_empty_frac, s.range_b= ounds_histogram "
+=C2=A0 =C2=A0"FROM pg_catalog.pg_stats AS s "
+=C2=A0 =C2=A0"WHERE s.schemaname =3D $1 AND s.tablename =3D $2 "=
+=C2=A0 =C2=A0"ORDER BY s.attname, s.inherited DESC";

I think we should retrieve the attribute statistics for only the
referenced columns of the remote table, not all the columns of it, to
reduce the data transfer and the cost of matching local/remote
attributes in import_fetched_statistics().

<= div>I thought about this, and decided that we wanted to 1) avoid per-column= round trips and 2) keep the remote queries simple. It's not such a big= deal to add "AND s.attname =3D ANY($3)" and construct a '{at= t1,att2,"ATt3"}' string, as we already do in pg_dump in a few= places.
=C2=A0

In fetch_remote_statistics()

+=C2=A0 =C2=A0if (server_version_num >=3D 180000)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0relation_sql =3D relstats_query_18;
+=C2=A0 =C2=A0else if (server_version_num >=3D 140000)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0relation_sql =3D relstats_query_14;
+=C2=A0 =C2=A0else
+=C2=A0 =C2=A0 =C2=A0 =C2=A0relation_sql =3D relstats_query_default;

I think that having the definition for each of relstats_query_18,
relstats_query_14 and relstats_query_default makes the code redundant
and the maintenance hard.=C2=A0 To avoid that, how about building the
relation_sql query dynamically as done for the query to fetch all
table data from the remote server in postgresImportForeignSchema().
Same for the attribute_sql query.

It ma= y be a moot point. If we're not fetching relallfrozen, then the 14 &= ; 18 cases are now the same, and since the pre-14 case concerns differentia= ting between analyzed and unanalyzed tables, we would just map that to 0 IF= we kept those stats, but we almost never would because an unanalyzed remot= e table would not have the attribute stats necessary to qualify as a good r= emote fetch. So we're down to just one static query.
=C2=A0

That's all I have for now.=C2=A0 I will continue to review the changes.=

Much appreciated.
--000000000000a43f770647871787--