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.94.2) (envelope-from ) id 1qiWvs-006iKO-47 for pgsql-hackers@arkaria.postgresql.org; Tue, 19 Sep 2023 09:22:12 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1qiWvq-008G0e-Ps for pgsql-hackers@arkaria.postgresql.org; Tue, 19 Sep 2023 09:22:10 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qiWvq-008FzT-Dx for pgsql-hackers@lists.postgresql.org; Tue, 19 Sep 2023 09:22:10 +0000 Received: from oss.nttdata.com ([49.212.34.109]) by magus.postgresql.org with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qiWvk-00654W-HU for pgsql-hackers@lists.postgresql.org; Tue, 19 Sep 2023 09:22:07 +0000 Received: from oss.nttdata.com (localhost [127.0.0.1]) by oss.nttdata.com (Postfix) with ESMTPA id 71650606AB; Tue, 19 Sep 2023 18:21:59 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at oss.nttdata.com MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=_644b821498a33431ca48e3be799c151a" Content-Transfer-Encoding: 7bit Date: Tue, 19 Sep 2023 18:21:59 +0900 From: bt23nguyent To: Nathan Bossart Cc: Daniel Gustafsson , Alvaro Herrera , pgsql-hackers@lists.postgresql.org Subject: Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set In-Reply-To: <20230915143827.GA1912920@nathanxps13> References: <202309151049.dmfsxc5b3wyf@alvherre.pgsql> <7035FAFE-7E91-4B31-AA72-75EE82DB3F7B@yesql.se> <20230915143827.GA1912920@nathanxps13> User-Agent: Roundcube Webmail/1.4.11 Message-ID: <85b9e69997bbd5d8302a3a99a2de246d@oss.nttdata.com> X-Sender: bt23nguyent@oss.nttdata.com List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --=_644b821498a33431ca48e3be799c151a Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=US-ASCII; format=flowed On 2023-09-15 23:38, Nathan Bossart wrote: > On Fri, Sep 15, 2023 at 02:48:55PM +0200, Daniel Gustafsson wrote: >>> On 15 Sep 2023, at 12:49, Alvaro Herrera >>> wrote: >>> >>> On 2023-Sep-15, Daniel Gustafsson wrote: >>> >>>> -basic_archive_configured(ArchiveModuleState *state) >>>> +basic_archive_configured(ArchiveModuleState *state, const char >>>> **errmsg) >>>> >>>> The variable name errmsg implies that it will contain the errmsg() >>>> data when it >>>> in fact is used for errhint() data, so it should be named >>>> accordingly. > > I have no objection to allowing this callback to provide additional > information, but IMHO this should use errdetail() instead of errhint(). > In > the provided patch, the new message explains how the module is not > configured. It doesn't hint at how to fix it (although presumably one > could figure that out pretty easily). > >>>> It's probably better to define the interface as >>>> ArchiveCheckConfiguredCB >>>> functions returning an allocated string in the passed pointer which >>>> the caller >>>> is responsible for freeing. > > That does seem more flexible. > >>> Also note that this callback is documented in archive-modules.sgml, >>> so >>> that needs to be updated as well. This also means you can't >>> backpatch >>> this change, or you risk breaking external software that implements >>> this >>> interface. >> >> Absolutely, this is master only for v17. > > +1 Thank you for all of your comments! They are all really constructive and I totally agree with the points you brought up. I have updated the patch accordingly. Please let me know if you have any further suggestions that I can improve more. Best regards, Tung Nguyen --=_644b821498a33431ca48e3be799c151a Content-Transfer-Encoding: base64 Content-Type: text/x-diff; name=v2-0001-Improve-log-message-output-of-basic-archive.patch Content-Disposition: attachment; filename=v2-0001-Improve-log-message-output-of-basic-archive.patch; size=5096 ZGlmZiAtLWdpdCBhL2NvbnRyaWIvYmFzaWNfYXJjaGl2ZS9iYXNpY19hcmNoaXZlLmMgYi9jb250 cmliL2Jhc2ljX2FyY2hpdmUvYmFzaWNfYXJjaGl2ZS5jCmluZGV4IDRkNzhjMzE4NTkuLjdiODY3 M2YzMzggMTAwNjQ0Ci0tLSBhL2NvbnRyaWIvYmFzaWNfYXJjaGl2ZS9iYXNpY19hcmNoaXZlLmMK KysrIGIvY29udHJpYi9iYXNpY19hcmNoaXZlL2Jhc2ljX2FyY2hpdmUuYwpAQCAtNDgsNyArNDgs NyBAQCB0eXBlZGVmIHN0cnVjdCBCYXNpY0FyY2hpdmVEYXRhCiBzdGF0aWMgY2hhciAqYXJjaGl2 ZV9kaXJlY3RvcnkgPSBOVUxMOwogCiBzdGF0aWMgdm9pZCBiYXNpY19hcmNoaXZlX3N0YXJ0dXAo QXJjaGl2ZU1vZHVsZVN0YXRlICpzdGF0ZSk7Ci1zdGF0aWMgYm9vbCBiYXNpY19hcmNoaXZlX2Nv bmZpZ3VyZWQoQXJjaGl2ZU1vZHVsZVN0YXRlICpzdGF0ZSk7CitzdGF0aWMgYm9vbCBiYXNpY19h cmNoaXZlX2NvbmZpZ3VyZWQoQXJjaGl2ZU1vZHVsZVN0YXRlICpzdGF0ZSwgY2hhciAqKmxvZ2Rl dGFpbCk7CiBzdGF0aWMgYm9vbCBiYXNpY19hcmNoaXZlX2ZpbGUoQXJjaGl2ZU1vZHVsZVN0YXRl ICpzdGF0ZSwgY29uc3QgY2hhciAqZmlsZSwgY29uc3QgY2hhciAqcGF0aCk7CiBzdGF0aWMgdm9p ZCBiYXNpY19hcmNoaXZlX2ZpbGVfaW50ZXJuYWwoY29uc3QgY2hhciAqZmlsZSwgY29uc3QgY2hh ciAqcGF0aCk7CiBzdGF0aWMgYm9vbCBjaGVja19hcmNoaXZlX2RpcmVjdG9yeShjaGFyICoqbmV3 dmFsLCB2b2lkICoqZXh0cmEsIEd1Y1NvdXJjZSBzb3VyY2UpOwpAQCAtMTU5LDkgKzE1OSwxNSBA QCBjaGVja19hcmNoaXZlX2RpcmVjdG9yeShjaGFyICoqbmV3dmFsLCB2b2lkICoqZXh0cmEsIEd1 Y1NvdXJjZSBzb3VyY2UpCiAgKiBDaGVja3MgdGhhdCBhcmNoaXZlX2RpcmVjdG9yeSBpcyBub3Qg YmxhbmsuCiAgKi8KIHN0YXRpYyBib29sCi1iYXNpY19hcmNoaXZlX2NvbmZpZ3VyZWQoQXJjaGl2 ZU1vZHVsZVN0YXRlICpzdGF0ZSkKK2Jhc2ljX2FyY2hpdmVfY29uZmlndXJlZChBcmNoaXZlTW9k dWxlU3RhdGUgKnN0YXRlLCBjaGFyICoqbG9nZGV0YWlsKQogewotCXJldHVybiBhcmNoaXZlX2Rp cmVjdG9yeSAhPSBOVUxMICYmIGFyY2hpdmVfZGlyZWN0b3J5WzBdICE9ICdcMCc7CisJaWYgKGFy Y2hpdmVfZGlyZWN0b3J5ID09IE5VTEwgfHwgYXJjaGl2ZV9kaXJlY3RvcnlbMF0gPT0gJ1wwJykK KyAgICB7CisgICAgICAgICpsb2dkZXRhaWwgPSBwc3RyZHVwKCJXQUwgYXJjaGl2aW5nIGZhaWxl ZCBiZWNhdXNlIGJhc2ljX2FyY2hpdmUuYXJjaGl2ZV9kaXJlY3RvcnkgaXMgbm90IHNldCIpOwor ICAgICAgICByZXR1cm4gZmFsc2U7CisgICAgfQorICAgIGVsc2UKKyAgICAgICAgcmV0dXJuIHRy dWU7CiB9CiAKIC8qCmRpZmYgLS1naXQgYS9kb2Mvc3JjL3NnbWwvYXJjaGl2ZS1tb2R1bGVzLnNn bWwgYi9kb2Mvc3JjL3NnbWwvYXJjaGl2ZS1tb2R1bGVzLnNnbWwKaW5kZXggNzA2NDMwN2Q5ZS4u ZjlmNjE3Nzg0NCAxMDA2NDQKLS0tIGEvZG9jL3NyYy9zZ21sL2FyY2hpdmUtbW9kdWxlcy5zZ21s CisrKyBiL2RvYy9zcmMvc2dtbC9hcmNoaXZlLW1vZHVsZXMuc2dtbApAQCAtMTAxLDcgKzEwMSw3 IEBAIHR5cGVkZWYgdm9pZCAoKkFyY2hpdmVTdGFydHVwQ0IpIChBcmNoaXZlTW9kdWxlU3RhdGUg KnN0YXRlKTsKICAgICBhc3N1bWVzIHRoZSBtb2R1bGUgaXMgY29uZmlndXJlZC4KIAogPHByb2dy YW1saXN0aW5nPgotdHlwZWRlZiBib29sICgqQXJjaGl2ZUNoZWNrQ29uZmlndXJlZENCKSAoQXJj aGl2ZU1vZHVsZVN0YXRlICpzdGF0ZSk7Cit0eXBlZGVmIGJvb2wgKCpBcmNoaXZlQ2hlY2tDb25m aWd1cmVkQ0IpIChBcmNoaXZlTW9kdWxlU3RhdGUgKnN0YXRlLCBjaGFyICoqbG9nZGV0YWlsKTsK IDwvcHJvZ3JhbWxpc3Rpbmc+CiAKICAgICBJZiA8bGl0ZXJhbD50cnVlPC9saXRlcmFsPiBpcyBy ZXR1cm5lZCwgdGhlIHNlcnZlciB3aWxsIHByb2NlZWQgd2l0aApAQCAtMTEyLDcgKzExMiw5IEBA IHR5cGVkZWYgYm9vbCAoKkFyY2hpdmVDaGVja0NvbmZpZ3VyZWRDQikgKEFyY2hpdmVNb2R1bGVT dGF0ZSAqc3RhdGUpOwogV0FSTklORzogIGFyY2hpdmVfbW9kZSBlbmFibGVkLCB5ZXQgYXJjaGl2 aW5nIGlzIG5vdCBjb25maWd1cmVkCiA8L3NjcmVlbj4KICAgICBJbiB0aGUgbGF0dGVyIGNhc2Us IHRoZSBzZXJ2ZXIgd2lsbCBwZXJpb2RpY2FsbHkgY2FsbCB0aGlzIGZ1bmN0aW9uLCBhbmQKLSAg ICBhcmNoaXZpbmcgd2lsbCBwcm9jZWVkIG9ubHkgd2hlbiBpdCByZXR1cm5zIDxsaXRlcmFsPnRy dWU8L2xpdGVyYWw+LgorICAgIGFyY2hpdmluZyB3aWxsIHByb2NlZWQgb25seSB3aGVuIGl0IHJl dHVybnMgPGxpdGVyYWw+dHJ1ZTwvbGl0ZXJhbD4uIFRoZQorICAgIGFyY2hpdmVyIG1heSBhbHNv IGVtaXQgdGhlIGRldGFpbCBleHBsYWluaW5nIGhvdyB0aGUgbW9kdWxlIGlzIG5vdCBjb25maWd1 cmVkCisgICAgdG8gdGhlIHNldmVyIGxvZyBpZiB0aGUgYXJjaGl2ZSBtb2R1bGUgaGFzIGFueS4g CiAgICA8L3BhcmE+CiAgIDwvc2VjdDI+CiAKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FyY2hp dmUvc2hlbGxfYXJjaGl2ZS5jIGIvc3JjL2JhY2tlbmQvYXJjaGl2ZS9zaGVsbF9hcmNoaXZlLmMK aW5kZXggMTU3Y2E0Yzc1MS4uYzk1N2ExOGVlNyAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvYXJj aGl2ZS9zaGVsbF9hcmNoaXZlLmMKKysrIGIvc3JjL2JhY2tlbmQvYXJjaGl2ZS9zaGVsbF9hcmNo aXZlLmMKQEAgLTIzLDcgKzIzLDcgQEAKICNpbmNsdWRlICJjb21tb24vcGVyY2VudHJlcGwuaCIK ICNpbmNsdWRlICJwZ3N0YXQuaCIKIAotc3RhdGljIGJvb2wgc2hlbGxfYXJjaGl2ZV9jb25maWd1 cmVkKEFyY2hpdmVNb2R1bGVTdGF0ZSAqc3RhdGUpOworc3RhdGljIGJvb2wgc2hlbGxfYXJjaGl2 ZV9jb25maWd1cmVkKEFyY2hpdmVNb2R1bGVTdGF0ZSAqc3RhdGUsIGNoYXIgKipsb2dkZXRhaWwp Owogc3RhdGljIGJvb2wgc2hlbGxfYXJjaGl2ZV9maWxlKEFyY2hpdmVNb2R1bGVTdGF0ZSAqc3Rh dGUsCiAJCQkJCQkJICAgY29uc3QgY2hhciAqZmlsZSwKIAkJCQkJCQkgICBjb25zdCBjaGFyICpw YXRoKTsKQEAgLTQzLDcgKzQzLDcgQEAgc2hlbGxfYXJjaGl2ZV9pbml0KHZvaWQpCiB9CiAKIHN0 YXRpYyBib29sCi1zaGVsbF9hcmNoaXZlX2NvbmZpZ3VyZWQoQXJjaGl2ZU1vZHVsZVN0YXRlICpz dGF0ZSkKK3NoZWxsX2FyY2hpdmVfY29uZmlndXJlZChBcmNoaXZlTW9kdWxlU3RhdGUgKnN0YXRl LCBjaGFyICoqbG9nZGV0YWlsKQogewogCXJldHVybiBYTG9nQXJjaGl2ZUNvbW1hbmRbMF0gIT0g J1wwJzsKIH0KZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3Bvc3RtYXN0ZXIvcGdhcmNoLmMgYi9z cmMvYmFja2VuZC9wb3N0bWFzdGVyL3BnYXJjaC5jCmluZGV4IDQ2YWYzNDk1NjQuLjJmZDFkMDNi MDkgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3Bvc3RtYXN0ZXIvcGdhcmNoLmMKKysrIGIvc3Jj L2JhY2tlbmQvcG9zdG1hc3Rlci9wZ2FyY2guYwpAQCAtMzkwLDcgKzM5MCw4IEBAIHBnYXJjaF9B cmNoaXZlckNvcHlMb29wKHZvaWQpCiAJCXsKIAkJCXN0cnVjdCBzdGF0IHN0YXRfYnVmOwogCQkJ Y2hhcgkJcGF0aG5hbWVbTUFYUEdQQVRIXTsKLQorCQkJY2hhciAgICAgICAqbG9nZGV0YWlsOwor CQkJCiAJCQkvKgogCQkJICogRG8gbm90IGluaXRpYXRlIGFueSBtb3JlIGFyY2hpdmUgY29tbWFu ZHMgYWZ0ZXIgcmVjZWl2aW5nCiAJCQkgKiBTSUdURVJNLCBub3IgYWZ0ZXIgdGhlIHBvc3RtYXN0 ZXIgaGFzIGRpZWQgdW5leHBlY3RlZGx5LiBUaGUKQEAgLTQxMCwxMCArNDExLDEzIEBAIHBnYXJj aF9BcmNoaXZlckNvcHlMb29wKHZvaWQpCiAKIAkJCS8qIGNhbid0IGRvIGFueXRoaW5nIGlmIG5v dCBjb25maWd1cmVkIC4uLiAqLwogCQkJaWYgKEFyY2hpdmVDYWxsYmFja3MtPmNoZWNrX2NvbmZp Z3VyZWRfY2IgIT0gTlVMTCAmJgotCQkJCSFBcmNoaXZlQ2FsbGJhY2tzLT5jaGVja19jb25maWd1 cmVkX2NiKGFyY2hpdmVfbW9kdWxlX3N0YXRlKSkKKwkJCQkhQXJjaGl2ZUNhbGxiYWNrcy0+Y2hl Y2tfY29uZmlndXJlZF9jYihhcmNoaXZlX21vZHVsZV9zdGF0ZSwgJmxvZ2RldGFpbCkpCiAJCQl7 CiAJCQkJZXJlcG9ydChXQVJOSU5HLAotCQkJCQkJKGVycm1zZygiYXJjaGl2ZV9tb2RlIGVuYWJs ZWQsIHlldCBhcmNoaXZpbmcgaXMgbm90IGNvbmZpZ3VyZWQiKSkpOworCQkJCQkJKGVycm1zZygi YXJjaGl2ZV9tb2RlIGVuYWJsZWQsIHlldCBhcmNoaXZpbmcgaXMgbm90IGNvbmZpZ3VyZWQiKSks CisJCQkJCQkobG9nZGV0YWlsICE9IE5VTEwpID8gZXJyZGV0YWlsKCIlcyIsIGxvZ2RldGFpbCkg OiAwKTsKKwkJCQlpZiAobG9nZGV0YWlsICE9IE5VTEwpCisJCQkJCXBmcmVlKGxvZ2RldGFpbCk7 CiAJCQkJcmV0dXJuOwogCQkJfQogCmRpZmYgLS1naXQgYS9zcmMvaW5jbHVkZS9hcmNoaXZlL2Fy Y2hpdmVfbW9kdWxlLmggYi9zcmMvaW5jbHVkZS9hcmNoaXZlL2FyY2hpdmVfbW9kdWxlLmgKaW5k ZXggNjc5Y2U1YTZkYi4uNDNiZDBmZjRjMiAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvYXJjaGl2 ZS9hcmNoaXZlX21vZHVsZS5oCisrKyBiL3NyYy9pbmNsdWRlL2FyY2hpdmUvYXJjaGl2ZV9tb2R1 bGUuaApAQCAtMzYsNyArMzYsNyBAQCB0eXBlZGVmIHN0cnVjdCBBcmNoaXZlTW9kdWxlU3RhdGUK ICAqIGFyY2hpdmUgbW9kdWxlcyBkb2N1bWVudGF0aW9uLgogICovCiB0eXBlZGVmIHZvaWQgKCpB cmNoaXZlU3RhcnR1cENCKSAoQXJjaGl2ZU1vZHVsZVN0YXRlICpzdGF0ZSk7Ci10eXBlZGVmIGJv b2wgKCpBcmNoaXZlQ2hlY2tDb25maWd1cmVkQ0IpIChBcmNoaXZlTW9kdWxlU3RhdGUgKnN0YXRl KTsKK3R5cGVkZWYgYm9vbCAoKkFyY2hpdmVDaGVja0NvbmZpZ3VyZWRDQikgKEFyY2hpdmVNb2R1 bGVTdGF0ZSAqc3RhdGUsIGNoYXIgKipsb2dkZXRhaWwpOwogdHlwZWRlZiBib29sICgqQXJjaGl2 ZUZpbGVDQikgKEFyY2hpdmVNb2R1bGVTdGF0ZSAqc3RhdGUsIGNvbnN0IGNoYXIgKmZpbGUsIGNv bnN0IGNoYXIgKnBhdGgpOwogdHlwZWRlZiB2b2lkICgqQXJjaGl2ZVNodXRkb3duQ0IpIChBcmNo aXZlTW9kdWxlU3RhdGUgKnN0YXRlKTs= --=_644b821498a33431ca48e3be799c151a--