On Wed, Jul 20, 2022 at 07:27:07AM -0500, Justin Pryzby wrote:
> Looks fine
Thanks for checking, applied.
--
Michael
signature.asc
Description: PGP signature
On Tue, Jul 19, 2022 at 01:13:34PM +0900, Michael Paquier wrote:
> > It looks like you named the table "toast_relfilenodes", but then also store
> > to it data for non-toast tables.
>
> How about naming that index_relfilenodes? One difference with what I
> posted previously and 5fb5b6 is the
On Mon, Jul 18, 2022 at 09:26:53PM -0500, Justin Pryzby wrote:
> Sorry, I meant to send this earlier..
No problem.
> It looks like you named the table "toast_relfilenodes", but then also store
> to it data for non-toast tables.
How about naming that index_relfilenodes? One difference with what
On Sun, Jul 17, 2022 at 10:58:26AM +0100, Simon Riggs wrote:
> Sounds great, looks fine. Thanks for your review.
Ok, cool. At the end, I have decided to split the tests and the main
patch into two different commits, as each is useful on its own. Doing
so also helps in seeing the difference of
Sorry, I meant to send this earlier..
On Sun, Jul 17, 2022 at 03:19:47PM +0900, Michael Paquier wrote:
> The second thing is test coverage. Using a REINDEX DATABASE/SYSTEM
> +my $catalog_toast_index = $node->safe_psql('postgres',
> + "SELECT indexrelid::regclass FROM pg_index WHERE indrelid
On Sun, 17 Jul 2022 at 07:19, Michael Paquier wrote:
>
> On Fri, Jul 15, 2022 at 06:21:22PM +0100, Simon Riggs wrote:
> > That's fixed it on the CFbot. Over to you, Michael. Thanks.
>
> Sure. I have looked over that, and this looks fine overall. I have
> made two changes though.
>
> if
On Fri, Jul 15, 2022 at 06:21:22PM +0100, Simon Riggs wrote:
> That's fixed it on the CFbot. Over to you, Michael. Thanks.
Sure. I have looked over that, and this looks fine overall. I have
made two changes though.
if (objectKind == REINDEX_OBJECT_SYSTEM &&
-
On Fri, 15 Jul 2022 at 12:25, Simon Riggs wrote:
>
> On Mon, 4 Jul 2022 at 08:00, Michael Paquier wrote:
> >
> > On Sun, Jul 03, 2022 at 05:41:31PM -0400, Tom Lane wrote:
> > > This is marked as Ready for Committer, but that seems unduly
> > > optimistic.
> >
> > Please note that patch authors
On Fri, 15 Jul 2022 at 15:03, Tom Lane wrote:
>
> Alvaro Herrera writes:
> > Ah, one theory is that the libpq_pipeline program is getting linked to
> > an installed version of libpq that doesn't contain the fixes. Maybe you
> > can do `ldd /path/to/libpq_pipeline` and see which copy of libpq.so
Alvaro Herrera writes:
> Ah, one theory is that the libpq_pipeline program is getting linked to
> an installed version of libpq that doesn't contain the fixes. Maybe you
> can do `ldd /path/to/libpq_pipeline` and see which copy of libpq.so it
> is picking up?
That's pronounced "otool -L" on
On 2022-Jul-15, Alvaro Herrera wrote:
> Not sure what to make of this. Maybe the fix in 054325c5eeb3 is not
> right, but I don't understand why it doesn't fail in the macOS machines
> in the buildfarm.
Ah, one theory is that the libpq_pipeline program is getting linked to
an installed version
On 2022-Jul-15, Alvaro Herrera wrote:
> > Yeh, repeated failures on MacOS Catalina with HEAD.
>
> Can you share the contents of src/test/modules/libpq_pipeline/tmp_check?
> Since these failures are not visible in the buildfarm (where we do have
> 11.0 as well as some 10.X versions of macOS),
On 2022-Jul-15, Simon Riggs wrote:
> > Test Summary Report
> > ---
> > t/001_libpq_pipeline.pl (Wstat: 512 Tests: 20 Failed: 2)
> > Failed tests: 9-10
> > Non-zero exit status: 2
> > Files=1, Tests=20, 4 wallclock secs ( 0.02 usr 0.00 sys + 0.66 cusr
> > 0.64 csys = 1.32
On Mon, 4 Jul 2022 at 08:00, Michael Paquier wrote:
>
> On Sun, Jul 03, 2022 at 05:41:31PM -0400, Tom Lane wrote:
> > This is marked as Ready for Committer, but that seems unduly
> > optimistic.
>
> Please note that patch authors should not switch a patch as RfC by
> themselves. This is
On Fri, 15 Jul 2022 at 12:15, Simon Riggs wrote:
>
> On Fri, 15 Jul 2022 at 09:12, Alvaro Herrera wrote:
> >
> > On 2022-Jul-15, Simon Riggs wrote:
> >
> > > On Fri, 15 Jul 2022 at 04:44, Michael Paquier wrote:
> >
> > > > This patch has been marked as waiting for a review, however the CF bot
>
On Fri, 15 Jul 2022 at 09:12, Alvaro Herrera wrote:
>
> On 2022-Jul-15, Simon Riggs wrote:
>
> > On Fri, 15 Jul 2022 at 04:44, Michael Paquier wrote:
>
> > > This patch has been marked as waiting for a review, however the CF bot
> > > is completely red:
> >
> > Yes, it is failing, but so is
On 2022-Jul-15, Simon Riggs wrote:
> On Fri, 15 Jul 2022 at 04:44, Michael Paquier wrote:
> > This patch has been marked as waiting for a review, however the CF bot
> > is completely red:
>
> Yes, it is failing, but so is current HEAD, with some kind of libpq
> pipelining error.
Hmm, is it?
On Fri, 15 Jul 2022 at 04:44, Michael Paquier wrote:
>
> On Mon, Jul 04, 2022 at 03:59:55PM +0900, Michael Paquier wrote:
> > Please note that patch authors should not switch a patch as RfC by
> > themselves. This is something that a reviewer should do.
>
>
On Mon, Jul 04, 2022 at 03:59:55PM +0900, Michael Paquier wrote:
> Please note that patch authors should not switch a patch as RfC by
> themselves. This is something that a reviewer should do.
This patch has been marked as waiting for a review, however the CF bot
is completely red:
On 2022-Jun-29, Simon Riggs wrote:
> > -if (strcmp(objectName, get_database_name(objectOid)) != 0)
> > +if (objectName && strcmp(objectName, get_database_name(objectOid))
> > != 0)
> > ereport(ERROR,
> > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>
On Sun, Jul 03, 2022 at 05:41:31PM -0400, Tom Lane wrote:
> This is marked as Ready for Committer, but that seems unduly
> optimistic.
Please note that patch authors should not switch a patch as RfC by
themselves. This is something that a reviewer should do.
> The cfbot shows that it's failing
Simon Riggs writes:
> Thanks for the review, new version attached.
This is marked as Ready for Committer, but that seems unduly
optimistic. The cfbot shows that it's failing on all platforms ---
and not in the same way on each, suggesting there are multiple
problems.
On Wed, 29 Jun 2022 at 05:35, Michael Paquier wrote:
>
> On Tue, Jun 28, 2022 at 11:02:25AM +0100, Simon Riggs wrote:
> > Attached patch is tested, documented and imho ready to be committed,
> > so I will mark it so in CFapp.
Thanks for the review Michael.
> The behavior introduced by this
On Tue, Jun 28, 2022 at 11:02:25AM +0100, Simon Riggs wrote:
> Attached patch is tested, documented and imho ready to be committed,
> so I will mark it so in CFapp.
The behavior introduced by this patch should be reflected in
reindexdb. See in particular reindex_one_database(), where a
On Tue, 28 Jun 2022 at 08:29, Simon Riggs wrote:
>
> On Thu, 2 Jun 2022 at 01:02, Michael Paquier wrote:
> >
> > On Tue, May 31, 2022 at 02:30:32PM +0200, Alvaro Herrera wrote:
> > > I was thinking the opposite: REINDEX DATABASE with or without a database
> > > name should always process the
On Thu, 2 Jun 2022 at 01:02, Michael Paquier wrote:
>
> On Tue, May 31, 2022 at 02:30:32PM +0200, Alvaro Herrera wrote:
> > I was thinking the opposite: REINDEX DATABASE with or without a database
> > name should always process the user relations and skip system catalogs.
> > If the user wants to
Maybe the answer is to 1) add a parenthesized option REINDEX(SYSTEM) (to allow
the current behavior); and 2) make REINDEX DATABASE an alias which implies
"SYSTEM false"; 3) prohibit REINDEX (SYSTEM true) SYSTEM, or consider removing
"REINDEX SYSTEM;".
That avoids the opaque and surprising
On Tue, May 31, 2022 at 02:30:32PM +0200, Alvaro Herrera wrote:
> I was thinking the opposite: REINDEX DATABASE with or without a database
> name should always process the user relations and skip system catalogs.
> If the user wants to do both, then they can use REINDEX SYSTEM in
> addition.
>
>
On 2022-May-31, Michael Paquier wrote:
> The case with CONCURRENTLY is different though: the option will never
> work on system catalogs so we have to skip them. Echoing with others
> on this thread, I don't think that we should introduce a different
> behavior on what's basically the same
On Tue, May 31, 2022 at 11:04:58AM +0200, Bernd Helmle wrote:
> And we already have a situation where this already happens with REINDEX
> DATABASE: if you use CONCURRENTLY, it skips system catalogs already and
> prints a warning. In both cases there are good technical reasons to
> skip catalog
Am Dienstag, dem 10.05.2022 um 15:00 +0100 schrieb Simon Riggs:
[...]
>
> > I think REINDEX DATABASE reindexing the current database is a good
> > usability improvement in itself. But skipping the shared catalogs
> > needs an explicity syntax. Not sure how feasible it is but
> > something
> >
Am Freitag, dem 27.05.2022 um 19:08 + schrieb Cary Huang:
[...]
> The patch applies and tests fine and I think this patch has good
> intentions to prevent the default behavior of REINDEX DATABASE to
> cause a deadlock. However, I am not in favor of simply omitting the
> database name after
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation:tested, passed
Hello
The patch applies and tests fine and I think this
On Wed, 11 May 2022 at 05:24, Ashutosh Bapat
wrote:
>> It is designed like this because it is dangerous to REINDEX the system
>> catalogs because of potential deadlocks, so we want a way to avoid
>> that problem.
>
> It's more clear if we add SKIP SYSTEM CATALOGS or some such explicit syntax.
Am Mittwoch, dem 11.05.2022 um 14:42 +0900 schrieb Michael Paquier:
> Agreed. Nobody is going to remember the difference. REINDEX's
> parsing grammar is designed to be extensible because we have the
> parenthesized flavor. Why don't you add an option there to skip the
> catalogs, like a
On Wed, May 11, 2022 at 09:54:17AM +0530, Ashutosh Bapat wrote:
> REINDEX DATABASE does system catalogs as well
> REINDEX DATABASE does everything except system catalogs
>
> That's confusing and unintuitive.
Agreed. Nobody is going to remember the difference. REINDEX's
parsing grammar is
On Tue, May 10, 2022 at 7:30 PM Simon Riggs
wrote:
> On Tue, 10 May 2022 at 14:47, Ashutosh Bapat
> wrote:
> >
> > On Tue, May 10, 2022 at 2:43 PM Simon Riggs
> > wrote:
> > >
> > > A minor issue, and patch.
> > >
> > > REINDEX DATABASE currently requires you to write REINDEX DATABASE
> > >
On Tue, 10 May 2022 at 14:47, Ashutosh Bapat
wrote:
>
> On Tue, May 10, 2022 at 2:43 PM Simon Riggs
> wrote:
> >
> > A minor issue, and patch.
> >
> > REINDEX DATABASE currently requires you to write REINDEX DATABASE
> > dbname, which makes this a little less usable than we might like.
> >
> >
On Tue, May 10, 2022 at 2:43 PM Simon Riggs
wrote:
>
> A minor issue, and patch.
>
> REINDEX DATABASE currently requires you to write REINDEX DATABASE
> dbname, which makes this a little less usable than we might like.
>
> REINDEX on the catalog can cause deadlocks, which also makes REINDEX
>
Hi,
Am Dienstag, dem 10.05.2022 um 10:13 +0100 schrieb Simon Riggs:
> A minor issue, and patch.
>
> REINDEX DATABASE currently requires you to write REINDEX DATABASE
> dbname, which makes this a little less usable than we might like.
>
> REINDEX on the catalog can cause deadlocks, which also
A minor issue, and patch.
REINDEX DATABASE currently requires you to write REINDEX DATABASE
dbname, which makes this a little less usable than we might like.
REINDEX on the catalog can cause deadlocks, which also makes REINDEX
DATABASE not much use in practice, and is the reason there is no test
41 matches
Mail list logo
