Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1gvcaK-0000R9-C7 for pgsql-hackers@arkaria.postgresql.org; Mon, 18 Feb 2019 06:39:24 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1gvcaI-000319-MR for pgsql-hackers@arkaria.postgresql.org; Mon, 18 Feb 2019 06:39:22 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1gvcaI-0002sR-4p for pgsql-hackers@lists.postgresql.org; Mon, 18 Feb 2019 06:39:22 +0000 Received: from mx0a-00296801.pphosted.com ([148.163.150.38]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1gvcaD-0005aX-MR for pgsql-hackers@postgresql.org; Mon, 18 Feb 2019 06:39:20 +0000 Received: from pps.filterd (m0114582.ppops.net [127.0.0.1]) by mx0a-00296801.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x1I6Wq0D003093 for ; Mon, 18 Feb 2019 06:39:16 GMT Received: from mail-lj1-f199.google.com (mail-lj1-f199.google.com [209.85.208.199]) by mx0a-00296801.pphosted.com with ESMTP id 2qp995h173-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Mon, 18 Feb 2019 06:39:15 +0000 Received: by mail-lj1-f199.google.com with SMTP id h14so3653188lja.11 for ; Sun, 17 Feb 2019 22:39:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=L2U6uzkclEbauInvMNhvT+jf3HR0Fr1n1jKDPbG0cao=; b=JF9TE68BHQ/XDtpXXFPmDdQ9W5E7C3DvPO5RDrDDOEYDzhxJghG4GTKPHjL72DeNuD hwcM373fkwfqqSw0600W4uIe+VTjDX8nA9gU+GnoyyyBnYmY1ArOckkDnmNF2OxB7cWD iqULeSQYKfWD76JkOednU2GHDBtO4UOC8Xm5v9XKeD/pwcNIgPwGrLRLvr1L+SuJRkx3 K8KiCr6OS3w5R9YZmDYEdygC6Oh6pVk9ew2gkILHJuNmp8ciYz74u3Yh/F2vpyPfQCe5 PBPiRgtZeMFgHBXyJMDjsr2pBXH+zCL6THm5m64lfuQSEZ8qcUy+6aDNymfbVjwVQKTq O2IA== X-Gm-Message-State: AHQUAuYpycQUW9eYR0VUnVziTzYWvu8fgTl48WfNdPipj5g14WzNPD+q PVIflZ/AGPrsBspA1JKdZE2anWm+eTn6hA4BvFXfKVywf98Yygf4Xgdjm33AJMCUTBkAmNI52UP Kq6fbXeCQHOU3I38I5mZ2rXdnoHpKuwd7ziRQRgZr1Cvo7tLH3+NLI6cOgYCsVfBOMQ== X-Received: by 2002:a2e:94ce:: with SMTP id r14-v6mr13068145ljh.34.1550471953094; Sun, 17 Feb 2019 22:39:13 -0800 (PST) X-Google-Smtp-Source: AHgI3IaA7hsjFryEz/kSPxW/Ibyjw6ollLPh6tjles94Y6fjhJLPR5KNAo6wdk5n9wzQuEk0Jzj/3IxctPMmrdgxiCs= X-Received: by 2002:a2e:94ce:: with SMTP id r14-v6mr13068126ljh.34.1550471952697; Sun, 17 Feb 2019 22:39:12 -0800 (PST) MIME-Version: 1.0 References: <20181226055647.GK2234@paquier.xyz> <20190216045351.q344ex2utwk7ndtr@alap3.anarazel.de> In-Reply-To: <20190216045351.q344ex2utwk7ndtr@alap3.anarazel.de> From: Hubert Zhang Date: Mon, 18 Feb 2019 14:39:04 +0800 Message-ID: Subject: Re: Control your disk usage in PG: Introduction to Disk Quota Extension To: Andres Freund Cc: Michael Paquier , Robert Haas , "pgsql-hackers@postgresql.org" , Tomas Vondra , Heikki Linnakangas Content-Type: multipart/alternative; boundary="000000000000f9c2480582256184" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-02-18_04:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1902180052 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000f9c2480582256184 Content-Type: text/plain; charset="UTF-8" Hi Andres On Sat, Feb 16, 2019 at 12:53 PM Andres Freund wrote: > Hi, > On 2019-01-30 10:26:52 +0800, Hubert Zhang wrote: > > Hi Michael, Robert > > For you question about the hook position, I want to explain more about > the > > background why we want to introduce these hooks. > > We wrote a diskquota extension < > https://github.com/greenplum-db/diskquota> > > [ ...] > > On Tue, Jan 22, 2019 at 12:08 PM Hubert Zhang wrote: > > > > > > For this particular purpose, I don't immediately see why you need a > > >> > hook in both places. If ReadBuffer is called with P_NEW, aren't we > > >> > guaranteed to end up in smgrextend()? > > >> Yes, that's a bit awkward. > > Please note that on PG lists the customary and desired style is to quote > inline in messages rather than top-quote. > > Greetings, > > Andres Freund > Thanks for your note. I will reply the above questions again. On Wed, Nov 21, 2018 at 10:48 PM Robert Haas wrote: > On Tue, Nov 20, 2018 at 2:20 AM Haozhou Wang wrote: > > We prepared a patch that includes the hook points. And such hook points > are needed for disk quota extension. > > There are two hooks. > > One is SmgrStat_hook. It's used to perform ad-hoc logic in storage when > doing smgr create/extend/truncate in general. As for disk quota extension, > this hook is used to detect active tables(new created tables, tables > extending new blocks, or tables being truncated) > > The other is BufferExtendCheckPerms_hook. It's used to perform ad-hoc > logic when buffer extend a new block. Since ReadBufferExtended is a hot > function, we call this hook only when blockNum == P_NEW. As for disk quota > extension, this hook is used to do query enforcement during the query is > loading data. > > > > Any comments are appreciated. > > +1 for adding some hooks to support this kind of thing, but I think > the names you've chosen are not very good. The hook name should > describe the place from which it is called, not the purpose for which > one imagines that it will be used, because somebody else might imagine > another use. Both BufferExtendCheckPerms_hook_type and > SmgrStat_hook_type are imagining that they know what the hook does - > CheckPerms in the first case and Stat in the second case. > > For this particular purpose, I don't immediately see why you need a > hook in both places. If ReadBuffer is called with P_NEW, aren't we > guaranteed to end up in smgrextend()? We have removed the hook in ReadBufferExtended and only keep the hooks in SMGR. On Wed, Dec 26, 2018 at 1:56 PM Michael Paquier wrote: > On Wed, Nov 21, 2018 at 09:47:44AM -0500, Robert Haas wrote: > > +1 for adding some hooks to support this kind of thing, but I think > > the names you've chosen are not very good. The hook name should > > describe the place from which it is called, not the purpose for which > > one imagines that it will be used, because somebody else might imagine > > another use. Both BufferExtendCheckPerms_hook_type and > > SmgrStat_hook_type are imagining that they know what the hook does - > > CheckPerms in the first case and Stat in the second case. > > I personally don't mind making Postgres more pluggable, but I don't > think that we actually need the extra ones proposed here at the layer > of smgr, as smgr is already a layer designed to call an underlying set > of APIs able to extend, unlink, etc. depending on the storage type. > > The reason to use smgr as the hook position is as follows: These hooks will be used by diskquota extension, which needs to gather the current disk usage for each schema. We want to use hooks to detect the Active Tables, and only recalculate the disk size of all the Active Tables. As you mentioned, smgr is the layer to call underlying API to extend/unlink files. So it's also the place to detect the table size change, i.e. the Active Tables. For example, the smgrextend hook indicates that you allocate a new block and the corresponding table needs to be treated as Active Table. Besides, suppose we use smgr as hook position, I want to discuss the API passed to the hook function. Take smgrextend as example. The function interface of smgrextend is like that: ``` void smgrextend (SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, char *buffer, bool skipFsync); ``` So the hook api of smgrextend could have two main options. Hook API Option1 ``` typedef void (*smgrextend_hook_type) (RelFileNode smgr_rnode,ForkNumber forknum); ``` Hook API Option 2 ``` typedef void (*smgrextend_hook_type) (SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,char *buffer, bool skipFsync); ``` As for Option1. Since diskquota extension only needs the relfilenode information to detect the active tables, Option1 just pass the RelFileNode to the hook function. It's more clear and has semantic meaning. While Option 2 is to pass the original parameters to the hook functions without any filter. This is more general and let the different hook implementations to decide how to use these parameters. Option 1 also needs some additional work to handle smgrdounlinkall case, whose input parameter is the SMgrRelation list. We may need to palloc Relfilenode list and pfree it manually. smgrdounlinkall function interface: ``` smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo, char *relstorages) ``` Based on the assumption we use smgr as hook position, hook API option1 or option2 which is better? Or we could find some balanced API between option1 and option2? Again comments on other better hook positions are also appreciated! -- Thanks Hubert Zhang --000000000000f9c2480582256184 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0Andres

On Sat, Feb 16, 2019 at 12:53 PM Andres Freund <andres@anarazel.de> wrote:
Hi,<= br> On 2019-01-30 10:26:52 +0800, Hubert Zhang wrote:
> Hi Michael, Robert
> For you question about the hook position, I want to explain more about= the
> background why we want to introduce these hooks.
> We wrote a diskquota extension <https://github.com/= greenplum-db/diskquota>
> [ ...]
> On Tue, Jan 22, 2019 at 12:08 PM Hubert Zhang <hzhang@pivotal.io> wrote:
>
> > > For this particular purpose, I don't immediately see why= you need a
> >> > hook in both places.=C2=A0 If ReadBuffer is called with = P_NEW, aren't we
> >> > guaranteed to end up in smgrextend()?
> >> Yes, that's a bit awkward.

Please note that on PG lists the customary and desired style is to quote inline in messages rather than top-quote.

Greetings,

Andres Freund

Thanks for your note. I will reply= the above questions again.

On Wed, Nov 21, 2018 at 10:48 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Nov 20, 2018 at 2:20 AM Haozhou Wang <hawang@pivotal.io> w= rote:
> We prepared a patch that includes the hook points. And such h= ook points are needed for disk quota extension.
> There are two hooks= .
> One is SmgrStat_hook. It's used to perform ad-hoc logic in st= orage when doing smgr create/extend/truncate in general. As for disk quota = extension, this hook is used to detect active tables(new created tables, ta= bles extending new blocks, or tables being truncated)
> The other is = BufferExtendCheckPerms_hook. It's used to perform ad-hoc logic when buf= fer extend a new block. Since ReadBufferExtended is a hot function, we call= this hook only when blockNum =3D=3D P_NEW. As=C2=A0 for disk quota extensi= on, this hook is used to do query enforcement during the query is loading d= ata.
>
> Any comments are appreciated.

+1 for adding som= e hooks to support this kind of thing, but I think
the names you've = chosen are not very good.=C2=A0 The hook name should
describe the place = from which it is called, not the purpose for which
one imagines that it = will be used, because somebody else might imagine
another use.=C2=A0 Bot= h BufferExtendCheckPerms_hook_type and
SmgrStat_hook_type are imagining = that they know what the hook does -
CheckPerms in the first case and Sta= t in the second case.

For this particular purpose, I don't immed= iately see why you need a
hook in both places.=C2=A0 If ReadBuffer is ca= lled with P_NEW, aren't we
guaranteed to end up in smgrextend()?

We have removed the hook in ReadBufferExtended= and only keep the hooks in SMGR.


<= div dir=3D"ltr" style=3D"color:rgb(80,0,80)">On Wed, Dec 26, 2018 at 1:56 P= M Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Nov 21, 2018 at 09:47:44A= M -0500, Robert Haas wrote:
> +1 for adding some hooks to support thi= s kind of thing, but I think
> the names you've chosen are not ve= ry good.=C2=A0 The hook name should
> describe the place from which i= t is called, not the purpose for which
> one imagines that it will be= used, because somebody else might imagine
> another use.=C2=A0 Both = BufferExtendCheckPerms_hook_type and
> SmgrStat_hook_type are imagini= ng that they know what the hook does -
> CheckPerms in the first case= and Stat in the second case.

I personally don't mind making Pos= tgres more pluggable, but I don't
think that we actually need the ex= tra ones proposed here at the layer
of smgr, as smgr is already a layer = designed to call an underlying set
of APIs able to extend, unlink, etc. = depending on the storage type.


The = reason to use smgr as the hook position is as follows:
Thes= e hooks will be used by diskquota extension, which needs to gather the curr= ent disk usage for each schema. We want to use hooks to detect the Active T= ables, and only recalculate the disk size of all the Active Tables. As you = mentioned, smgr is the layer to call underlying API to extend/unlink files.= So it's also the place to detect the table size change, i.e. the Activ= e Tables.
For example, the smgrextend hook indicates that you all= ocate a new block and the corresponding table needs to be treated as Active= Table.


Besides, suppose we us= e smgr as hook position, I want to discuss the API passed to the hook funct= ion.
Take smgrextend as example. The function interface of smgrex= tend is like that:
```
void smgrextend
(SMgrRelation reln, Fork= Number forknum, BlockNumber blocknum, char *buffer, bool skipFsync);
```=
So the hook api of smgrextend could have two main options.
Hook API Option1=C2=A0
```
typedef void (*smgrextend_hook= _type)
(RelFileNode smgr_rnode,ForkNumber forknum);
```
Hook API= Option 2
```
typedef void (*smgrextend_hook_type)
(= SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,char *buffer, b= ool skipFsync);
```

As for Opti= on1. Since diskquota extension only needs the relfilenode information to de= tect the active tables, Option1 just pass the RelFileNode to the hook funct= ion. It's more clear and has semantic meaning.

While Option 2 is to pass the original parameters to the hook functions wi= thout any filter. This is more general and let the different hook implement= ations to decide how to use these parameters.=C2=A0

Option 1 also needs some additional work to handle smgrdounlinkall case, = whose input parameter is the SMgrRelation list. We may need to palloc Relfi= lenode list and pfree it manually.=C2=A0
smgrdounlinkall function= interface:
```
smgrdounlinkall(SMgrRelation *rels, int nrel= s, bool isRedo, char *relstorages)
```

Based o= n the assumption we use smgr as hook position, hook API option1 or option2 = which is better?
Or we could find some balanced API between optio= n1 and option2?

Again comments on other better hoo= k positions are also appreciated!


= --
Thanks
Hubert Zhang
--000000000000f9c2480582256184--