MIME-Version: 1.0
Date: Thu, 08 Jun 2023 10:39:30 +0300
From: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
To: "Fujii.Yuki@df.MitsubishiElectric.co.jp"
 <Fujii.Yuki@df.mitsubishielectric.co.jp>
Cc: Bruce Momjian <bruce@momjian.us>, PostgreSQL-development
 <pgsql-hackers@postgresql.org>, Andres Freund <andres@anarazel.de>, Tom Lane
 <tgl@sss.pgh.pa.us>, Tomas Vondra <tomas.vondra@enterprisedb.com>, Julien
 Rouhaud <rjuju123@gmail.com>, Daniel Gustafsson <daniel@yesql.se>, Ilya
 Gladyshev <i.gladyshev@postgrespro.ru>
Subject: Re: Partial aggregates pushdown
In-Reply-To: <OS3PR01MB66607A988602D953D37482389553A@OS3PR01MB6660.jpnprd01.prod.outlook.com>
References: <OS3PR01MB6660A1DAF6619B9A0BF7D39B95E19@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <ZCYeIeFRgL2uBeuk@momjian.us>
 <OS3PR01MB6660B900F33523AAD0370D62958F9@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <ZC95C0+PVhVP3iax@momjian.us>
 <OSZPR01MB6662875933E87F37C711B4FC95969@OSZPR01MB6662.jpnprd01.prod.outlook.com>
 <ZDDIU2fJxZm/k5kj@momjian.us>
 <OS3PR01MB66606C1DA960336516D9A08395959@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <ZDedXNarI32SmN06@momjian.us> <ZDemPMLOyXNzKkhw@momjian.us>
 <OS3PR01MB6660EBFFFA06646F402813A395989@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <ZDj4nwJANUU+kam8@momjian.us>
 <OS3PR01MB66607DC466F7F3B3909DA3CA954EA@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <8ffedecae0d21a3cd93805baf1dcc0de@postgrespro.ru>
 <OS3PR01MB66604E404B003A621B0C03A69552A@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <93fc7f04f8c6978296c05f4f70671a43@postgrespro.ru>
 <OS3PR01MB666009200E5A3659C09955249552A@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <36f0ba8bf12a7285ca6ff51559f700db@postgrespro.ru>
 <OS3PR01MB66607A988602D953D37482389553A@OS3PR01MB6660.jpnprd01.prod.outlook.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <0a3960c36e705dccfce667f04f0bf632@postgrespro.ru>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/0a3960c36e705dccfce667f04f0bf632%40postgrespro.ru>
Precedence: bulk

Fujii.Yuki@df.MitsubishiElectric.co.jp писал 2023-06-08 02:08:
>> From: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
>> Sent: Wednesday, June 7, 2023 6:47 PM
>> This seems to be more robust, but the interface became more strange.
>> I'm not sure what to do with it. Some ideas I had to avoid introducing 
>> this
>> parameter. Not sure I like any of them.
>> 
>> 1) You can use QualifiedNameGetCreationNamespace() for 
>> aggpartialfnName
>> and still compare namespace and function name  for it and  aggName,
>> aggNamespace.
>> Seems to be not ideal, but avoids introducing new parameters.
>> 
>> 2) You can lookup for partial aggregate function after 
>> ProcedureCreate() in
>> AggregateCreate(), if it wasn't found at earlier stages. If it is the 
>> aggregate itself
>> - check it. If it's still not found, error out. Also seems to be a bit 
>> ugly - you leave
>> uncommitted garbage for vacuum in catalogue.
> Thank you for suggesting alternatives.
> The disadvantages of alternative 2) appear to be undesirable,
> I have modified it according to alternative 1)
> 
>> Another issue - the patch misses recording dependency between 
>> aggpartialfn
>> and aggregate procedure.
> I added code to record dependencys between aggpartialfn
> and aggregate procedure, similar to the code for functions such as 
> combinefunc.
> 

Hi.

Looks better. The only question I have is should we record dependency 
between procOid and aggpartialfn if aggpartialfn == procOid.

Also it seems new code likely should be run through pgindent.

doc/src/sgml/postgres-fdw.sgml:

+   For <literal>WHERE</literal> clauses,
+   <literal>JOIN</literal> clauses, this sending is active if
+   conditions in <xref 
linkend="postgres-fdw-remote-query-optimization"/>
+   hold and <varname>enable_partitionwise_join</varname> is true(this 
condition
+   is need for only <literal>JOIN</literal> clauses).
+   For aggregate expressions, this sending is active if conditions in

No space between "true" and "(" in "is true(this condition".

Some sentences in documentation, like one starting with
"For aggregate expressions, this sending is active if conditions in..."
seem to be too long, but I'm not the best man to read out documentation.

In "Built-in sharding in PostgreSQL" term "shard" doesn't have a 
definition.

By the way, I'm not sure that "sharding" documentation belongs to this 
patch,
at least it needs a review from native speaker.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional