From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Pg Hackers <pgsql-hackers@postgresql.org>,
 Ranier Vilela <ranier.vf@gmail.com>
Cc: David Rowley <dgrowleyml@gmail.com>
Subject: Re: Add proper planner support for ORDER BY / DISTINCT aggregates
Date: Mon, 05 Jul 2021 17:07:27 +0200
Message-ID: <3730969.3kmaB5N0Mh@aivenronan>
Organization: aiven
In-Reply-To: 
 <CAEudQArZmwkahm7pjy3_nKn2CRBZ1Dhf4qtT+J4wkdVUD8yfpA@mail.gmail.com>
References: 
 <CAEudQArZmwkahm7pjy3_nKn2CRBZ1Dhf4qtT+J4wkdVUD8yfpA@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Archived-At: 
 <https://www.postgresql.org/message-id/3730969.3kmaB5N0Mh%40aivenronan>
Precedence: bulk

Le lundi 5 juillet 2021, 16:51:59 CEST Ranier Vilela a =E9crit :
> >Please find attached a POC patch to do just that.
> >
> >The switch to the single-datum tuplesort is done when there is only one
> >attribute, it is byval (to avoid having to deal with copy of the
>=20
> references
>=20
> >everywhere) and we are not in bound mode (to also avoid having to move
>=20
> things
>=20
> >around).
>=20
> Hi, nice results!
>=20
> I have a few suggestions and questions to your patch:

Thank you for those !
>=20
> 1. Why do you moved the declaration of variable *plannode?
> I think this is unnecessary, extend the scope.

Sorry, I should have cleaned it up before sending.

>=20
> 2. Why do you declare a new variable TupleDesc out_tuple_desc at
> ExecInitSort?
> I think this is unnecessary too, maybe I didn't notice something.

Same as the above, thanks for the two.
>=20
> 3. I inverted the order of check at this line, I think "!node-bounded" is
> more cheap that TupleDescAttr(tupDesc, 0) ->attbyval

I'm not sure it matters since it's done once per sort but Ok
>=20
> 4. Once that you changed the order of execution, this test is unlikely th=
at
> happens, so add unlikely helps the branch.

Ok.

>=20
> 5. I think that you add a invariant inside the loop
> "if(node->is_single_val)"?
> Would not be better two fors?

Ok for me.

>=20
> For you convenience, I attached a v2 version (with styles changes), please
> take a look and can you repeat yours tests?

Tested it quickly, and did not see any change performance wise that cannot =
be=20
attributed to noise on my laptop but it's fine.

Thank you for the fixes !

>=20
> regards,
> Ranier Vilela


=2D-=20
Ronan Dunklau