public inbox for [email protected]
help / color / mirror / Atom feedNext commitfest app release is planned for March 18th
30+ messages / 9 participants
[nested] [flat]
* Next commitfest app release is planned for March 18th
@ 2025-03-04 01:21 Jelte Fennema-Nio <[email protected]>
0 siblings, 5 replies; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-04 01:21 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
The next commitfest app release is planned for March 18th and will
contain the following changes:
1. Major change: The homepage is revamped completely! It now shows a
dashboard of open patches where you are author/reviewer/committer if
you are logged in. These patches are ordered & grouped in a hopefully
useful way. If you're not logged in it will show you the current
commitfest. See screenshot for an example. The old list of all
commitfests is moved to the /archive (which has a button on the
homepage). Peter Geoghegan suggested adding a "dashboard" of this
kind. Feedback on this is very welcome, but depending on the
complexity I don't know when I'll get to it. I'll be a bit more busy
the next few weeks and also have some holidays planned.
2. Show name of a committer in the "Committer" column (instead of only
the username).
3. Fix the "Review" form so that all checkboxes can actually be
clicked. Thanks to Maciek.
4. Allow sorting patches by "failing since", this can be done by
clicking the header. This does *not* work on the staging website,
because CFbot is not sending CI updates there currently.
5. Remove "latest activity" column. This did not contain useful information.
6. The "latest email" column now shows "time since" (e.g. 1 week ago)
instead of an exact timestamp. You can still see the exact timestamp
by hovering over the cell.
7. Searching patches by author/reviewer now isn't a dropdown with a
ton of options, but instead has become a dropdown with a search box.
This also greatly improves page load performance: By not putting all
users in the HTML as a dropdown option it's saving 600-700ms in my
testing on staging.
8. Bugfix: Correctly show CI timeout as failure.
One thing I'm wondering about 3 though: Do people actually think these
checkboxes are even useful in the first place? For people not
familiar, they add these lines to an email:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: not tested
> Documentation: tested, passed
At least the first one seems not very useful, now that we have the
CFBot. Is the rest useful to anyone or do these buttons just result in
clutter.
As always, please test out the current staging website[1] to give some feedback.
HTTP auth user and password are both pgtest.
Also I wanted to highlight the work Jacob Brazeal is doing again. He
has been working on an AI-powered summarization and patch review
recommendation engine[2]. I definitely recommend people to take a look
at that and leave some feedback.
[1]: https://commitfest-test.postgresql.org/
[2]: https://patchwork-three.vercel.app/
Attachments:
[image/png] new-commitfest-homepage.png (387.6K, ../../CAGECzQR8oAaA2dFbE76-0s3f-Dgb4BJ4h1piux99QkHcmYqdTQ@mail.gmail.com/2-new-commitfest-homepage.png)
download | view image
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 09:22 Peter Eisentraut <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
4 siblings, 1 reply; 30+ messages in thread
From: Peter Eisentraut @ 2025-03-04 09:22 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On 04.03.25 02:21, Jelte Fennema-Nio wrote:
> 1. Major change: The homepage is revamped completely! It now shows a
> dashboard of open patches where you are author/reviewer/committer if
> you are logged in. These patches are ordered & grouped in a hopefully
> useful way. If you're not logged in it will show you the current
> commitfest. See screenshot for an example. The old list of all
> commitfests is moved to the /archive (which has a button on the
> homepage). Peter Geoghegan suggested adding a "dashboard" of this
> kind. Feedback on this is very welcome, but depending on the
> complexity I don't know when I'll get to it. I'll be a bit more busy
> the next few weeks and also have some holidays planned.
I don't know if I like that. I can see the point of getting to the
action quicker, but this sort of obscures the hierarchy of the site and
the data. Before it was like, select a commitfest, select a filter,
here are some patches. Now it's like, here is some stuff. Where did it
come from, how does it relate to the other stuff, how do I get to an
overview of all the stuff and the hierarchy of stuff.
How does one get back to the old homepage? I figured it out, you click
the "Archive" button. Why is that a button? Also, the row of buttons
is now seemingly a mix of actions on the current commit fest mixed with
site navigation. See above, what is the hierarchy of information and
the context of actions. This is a bit confusing.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 10:33 Jelte Fennema-Nio <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-04 10:33 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, 4 Mar 2025 at 10:22, Peter Eisentraut <[email protected]> wrote:
> I don't know if I like that. I can see the point of getting to the
> action quicker, but this sort of obscures the hierarchy of the site and
> the data. Before it was like, select a commitfest, select a filter,
> here are some patches. Now it's like, here is some stuff. Where did it
> come from, how does it relate to the other stuff, how do I get to an
> overview of all the stuff and the hierarchy of stuff.
I'm curious if there was anything specific that you used the old
homepage for. Especially things you did often that are now harder to
do. The only things I used on the homepage were:
1. Going to the "In Progress" and "Open" commitfest (usually with one
of the links that filter for patches related to me).
2. Going to the most-recently "Closed" commitfest to move/close my
previously submitted patches.
3. Search for commitfest entries by Message-ID
I agree that the new homepage now hides the hierarchy of the site, but
I'd say that most people using it probably don't really have to know
about that hierarchy. At least not care so much that it should be on
the initial page. I definitely have never clicked on a link for a
commitfest that's older than a year.
> How does one get back to the old homepage? I figured it out, you click
> the "Archive" button. Why is that a button? Also, the row of buttons
> is now seemingly a mix of actions on the current commit fest mixed with
> site navigation.
That's primarily to mirror the style of the commitfest page a bit.
i.e. Most of the buttons there are also links, i.e. "New patch" and
all the items under "Shortcuts"
> See above, what is the hierarchy of information and
> the context of actions. This is a bit confusing.
I do agree that it would be nicer to separate them. I'll look into
improving/replacing the navigation bar. But do you think these buttons
are so confusing, that this new homepage should be blocked on that?
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 10:34 Álvaro Herrera <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
4 siblings, 3 replies; 30+ messages in thread
From: Álvaro Herrera @ 2025-03-04 10:34 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On 2025-Mar-04, Jelte Fennema-Nio wrote:
> 1. Major change: The homepage is revamped completely! It now shows a
> dashboard of open patches where you are author/reviewer/committer if
> you are logged in. These patches are ordered & grouped in a hopefully
> useful way. If you're not logged in it will show you the current
> commitfest. See screenshot for an example. The old list of all
> commitfests is moved to the /archive (which has a button on the
> homepage).
I think showing different pages on the same URL depending on whether
you're logged in or not is not great UX. The idea of this dashboard
sounds very good to me, but it shouldn't replace the initial page (list
of commitfests). Maybe put this in a URL such as
https://commitfest.postgresql.org/you
or
https://commitfest.postgresql.org/me
or something easily reachable like that.
This also allows you to provide a standardized URL for one to look at
the activities of others, so if I visit
https://commitfest.postgresql.org/you/petere
I can see Peter's stuff.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Tom: There seems to be something broken here.
Teodor: I'm in sackcloth and ashes... Fixed.
http://postgr.es/m/[email protected]
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 12:35 Amit Kapila <[email protected]>
parent: Álvaro Herrera <[email protected]>
2 siblings, 1 reply; 30+ messages in thread
From: Amit Kapila @ 2025-03-04 12:35 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, Mar 4, 2025 at 4:05 PM Álvaro Herrera <[email protected]> wrote:
>
> On 2025-Mar-04, Jelte Fennema-Nio wrote:
>
> > 1. Major change: The homepage is revamped completely! It now shows a
> > dashboard of open patches where you are author/reviewer/committer if
> > you are logged in. These patches are ordered & grouped in a hopefully
> > useful way. If you're not logged in it will show you the current
> > commitfest. See screenshot for an example. The old list of all
> > commitfests is moved to the /archive (which has a button on the
> > homepage).
>
> I think showing different pages on the same URL depending on whether
> you're logged in or not is not great UX.
>
+1. The default should be what we see today, and there should be some
way to see the patches in which a particular person is involved.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 13:30 Jelte Fennema-Nio <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-04 13:30 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, 4 Mar 2025 at 13:36, Amit Kapila <[email protected]> wrote:
>
> On Tue, Mar 4, 2025 at 4:05 PM Álvaro Herrera <[email protected]> wrote:
> > I think showing different pages on the same URL depending on whether
> > you're logged in or not is not great UX.
> >
>
> +1. The default should be what we see today, and there should be some
> way to see the patches in which a particular person is involved.
I'm quite surprised that people seem to love the content of the
current homepage so much. Could someone explain why they want to see
this full list of commitfests as the first page you see? I feel like
I'm missing something here.
To be clear, I'm totally fine with moving the dashboard to a different
URL, definitely for now at least. But I personally would like to have
a homepage that shows me information that I come to the site to see,
not just a bunch of links to which a new link gets added each month.
Also having a homepage that shows completely different info for logged
in users and non-logged in users is pretty common. e.g. GitHub shows
you a similar dashboard when you're logged in, but shows a page with
generic information and a signup/signin link when you're not.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 14:26 Robert Haas <[email protected]>
parent: Álvaro Herrera <[email protected]>
2 siblings, 0 replies; 30+ messages in thread
From: Robert Haas @ 2025-03-04 14:26 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, Mar 4, 2025 at 5:35 AM Álvaro Herrera <[email protected]> wrote:
> The idea of this dashboard
> sounds very good to me, but it shouldn't replace the initial page (list
> of commitfests). Maybe put this in a URL such as
> https://commitfest.postgresql.org/you
> or
> https://commitfest.postgresql.org/me
> or something easily reachable like that.
+1 for this design.
I did think putting something on the home page was reasonable when I
first heard about it, but I think this is a better idea.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 16:15 Peter Eisentraut <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Peter Eisentraut @ 2025-03-04 16:15 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On 04.03.25 11:33, Jelte Fennema-Nio wrote:
> I'm curious if there was anything specific that you used the old
> homepage for. Especially things you did often that are now harder to
> do. The only things I used on the homepage were:
> 1. Going to the "In Progress" and "Open" commitfest (usually with one
> of the links that filter for patches related to me).
> 2. Going to the most-recently "Closed" commitfest to move/close my
> previously submitted patches.
> 3. Search for commitfest entries by Message-ID
>
> I agree that the new homepage now hides the hierarchy of the site, but
> I'd say that most people using it probably don't really have to know
> about that hierarchy. At least not care so much that it should be on
> the initial page. I definitely have never clicked on a link for a
> commitfest that's older than a year.
I think the option of having a list of things that I'm involved in as an
author *or* reviewer is actually very useful and something I have wanted
from time to time. But that is apparently not accessible using the
normal search/filter mechanism, because that is *and*. If that were
somehow available, then I could just bookmark something like
commitfest.postgresql.org/current/?author=-3&reviewer=-3&option=or
You could even, if people like this overall idea, make this a redirect
from commitfest.postgresql.org/. Because then I have context and this
makes sense in the hierarchy of the site, and I can work from there to
adjust the filters.
The problem now is that the home page is a unicorn. You can't get to
that listing in any other way, and you can't make similar listing by
starting from that listing and making adjustments to the filter.
I still think, however, that the homepage should provide overview and
not bombard you with too much content immediately. What is a
commitfest, which commitfest exists, how many have existed, when is the
next one, is there a next one, I think that helps people get context.
After all, we want them to get used to the system and stick around.
Another possible concern is that if you log in and have no patches and
have not signed up to review anything, then the default listing just
show you nothing?
>> How does one get back to the old homepage? I figured it out, you click
>> the "Archive" button. Why is that a button? Also, the row of buttons
>> is now seemingly a mix of actions on the current commit fest mixed with
>> site navigation.
>
> That's primarily to mirror the style of the commitfest page a bit.
> i.e. Most of the buttons there are also links, i.e. "New patch" and
> all the items under "Shortcuts"
Ok, but "New patch" still feels like an action, and "Shortcuts" is a
drop-down. But "Archive" is really just a link to a different page
without any (current or future) state changes. You could also imagine
"Archive" as an action, like "archive this", in which case a button
would be more appropriate.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 16:31 Peter Geoghegan <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
4 siblings, 1 reply; 30+ messages in thread
From: Peter Geoghegan @ 2025-03-04 16:31 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Mon, Mar 3, 2025 at 8:21 PM Jelte Fennema-Nio <[email protected]> wrote:
> 1. Major change: The homepage is revamped completely! It now shows a
> dashboard of open patches where you are author/reviewer/committer if
> you are logged in. These patches are ordered & grouped in a hopefully
> useful way. If you're not logged in it will show you the current
> commitfest. See screenshot for an example. The old list of all
> commitfests is moved to the /archive (which has a button on the
> homepage).
This looks very much like what I had in mind. Thanks!
> Peter Geoghegan suggested adding a "dashboard" of this
> kind. Feedback on this is very welcome, but depending on the
> complexity I don't know when I'll get to it. I'll be a bit more busy
> the next few weeks and also have some holidays planned.
But here you say that you *haven't* worked on what I had in mind,
which is confusing.
Are you saying that you have yet to implement a version of this that
shows everything (every patch that isn't closed out) for all
commitfests? What you've come up with only works for the current
commitfest, and not the next one?
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 17:02 Jelte Fennema-Nio <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-04 17:02 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, 4 Mar 2025 at 17:31, Peter Geoghegan <[email protected]> wrote:
> > Peter Geoghegan suggested adding a "dashboard" of this
> > kind. Feedback on this is very welcome, but depending on the
> > complexity I don't know when I'll get to it. I'll be a bit more busy
> > the next few weeks and also have some holidays planned.
>
> But here you say that you *haven't* worked on what I had in mind,
> which is confusing.
>
> Are you saying that you have yet to implement a version of this that
> shows everything (every patch that isn't closed out) for all
> commitfests? What you've come up with only works for the current
> commitfest, and not the next one?
I meant to say: I roughly implemented what Peter G described... Ideas
to further improve it are very welcome, but it might take a while
until I'm able to do that.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 17:25 Peter Geoghegan <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Peter Geoghegan @ 2025-03-04 17:25 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, Mar 4, 2025 at 12:02 PM Jelte Fennema-Nio <[email protected]> wrote:
>
> On Tue, 4 Mar 2025 at 17:31, Peter Geoghegan <[email protected]> wrote:
> > > Peter Geoghegan suggested adding a "dashboard" of this
> > > kind. Feedback on this is very welcome, but depending on the
> > > complexity I don't know when I'll get to it. I'll be a bit more busy
> > > the next few weeks and also have some holidays planned.
> >
> > But here you say that you *haven't* worked on what I had in mind,
> > which is confusing.
> >
> > Are you saying that you have yet to implement a version of this that
> > shows everything (every patch that isn't closed out) for all
> > commitfests? What you've come up with only works for the current
> > commitfest, and not the next one?
>
> I meant to say: I roughly implemented what Peter G described... Ideas
> to further improve it are very welcome, but it might take a while
> until I'm able to do that.
Got it.
From the looks of the screen shot that you posted (can't seem to find
the same dashboard view on https://commitfest-test.postgresql.org?),
this is *exactly* what I had in mind -- I don't know what I said that
you haven't fully taken into account here? It's just a screen shot,
but as far as it goes it looks great.
Did you mean that you have general doubts about the general quality of
the dashboard code? As in, the code itself is rough? Seems unlikely
that that was what you meant, since you also seemed to say that you're
planning another release (i.e. deploying to production) on March 18.
I'm fairly neutral on the question of whether or not the homepage
should just be the dashboard for logged in users. Though I do think
it's important that the new dashboard is highly discoverable, so that
people know that it exists without having to be told about it.
It'd be nice if at some point you also added the ability to
star/favorite/like patches -- I'm thinking of something that worked a
little bit like starring a gmail thread. Any such patches would appear
towards the end of the dashboard page, in its own section,
independently of whether I as a user am involved or not involved in
the patch. This would be private information, visible only to the
individual user that favorited the patch -- a mere bookmark.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 20:06 Jelte Fennema-Nio <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-04 20:06 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, 4 Mar 2025 at 18:25, Peter Geoghegan <[email protected]> wrote:
> From the looks of the screen shot that you posted (can't seem to find
> the same dashboard view on https://commitfest-test.postgresql.org?),
The dashboard is only available if you login. You probably have to
create an account to do so, because the staging auth and prod auth
systems are separate. Then you can mark yourself as author/reviewer of
a few patches to see what it would look like.
> this is *exactly* what I had in mind -- I don't know what I said that
> you haven't fully taken into account here? It's just a screen shot,
> but as far as it goes it looks great.
I think I grouped & ordered things slightly different than you
described. There are 4 groups (all of which are in the screenshot).
And then within each group patches are ordered like this:
1. Lowest max of "failing since", "needs rebase since", "time since
its commitfest was closed" at the top. NULLs (i.e. healthy patches)
are first.
2. If 1 tied (usually nulls) ordered by their commitfest startdate
(most recent startdate first)
3. If 2 ties, then patches are ordered by "most recent email"
So patches with failing CI in the "in progress cf" will sort below
healthy patches in the "open cf". I don't think you necessarily said
that, but this seemed nice to me. And it's easy to spot which patches
are for which CF because of the color coded CF labels.
This kind of sorting is possibly worth tweaking a bit after people
start using this and running into annoyances or unexpected sorts in
practice. Some other thing that's missing is the ability to "filter by
commitfest".
> Did you mean that you have general doubts about the general quality of
> the dashboard code? As in, the code itself is rough?
Nah, that's not what I meant.
> It'd be nice if at some point you also added the ability to
> star/favorite/like patches -- I'm thinking of something that worked a
> little bit like starring a gmail thread. Any such patches would appear
> towards the end of the dashboard page, in its own section,
> independently of whether I as a user am involved or not involved in
> the patch. This would be private information, visible only to the
> individual user that favorited the patch -- a mere bookmark.
Yeah, I had similar ideas.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 20:37 Jelte Fennema-Nio <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-04 20:37 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, 4 Mar 2025 at 17:15, Peter Eisentraut <[email protected]> wrote:
> I think the option of having a list of things that I'm involved in as an
> author *or* reviewer is actually very useful and something I have wanted
> from time to time. But that is apparently not accessible using the
> normal search/filter mechanism, because that is *and*.
Yeah I totally agree. It's not so simple as to just add "or" though,
because you would still want e.g. a filter by a patch status to be an
AND.
> The problem now is that the home page is a unicorn.
There are two reasons I did that:
1. This new homepage includes open patches from *all* commitfests. And
there's currently no page with that information.
2. I didn't want to risk breaking people's existing workflows in this
final CF of the year. Once people are happy with it, I definitely plan
to also add sorting/filtering (as an option) to the normal commitfest
pages.
> and you can't make similar listing by
> starting from that listing and making adjustments to the filter.
I think you're either wrong, or I misunderstand what you meant here.
You definitely can filter patches on the homepage using the same
filter controls. It's just AND again on top of the homepage filters.
> Another possible concern is that if you log in and have no patches and
> have not signed up to review anything, then the default listing just
> show you nothing?
Yeah, that's bad. It should at least show some info in that case.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 20:42 Jelte Fennema-Nio <[email protected]>
parent: Álvaro Herrera <[email protected]>
2 siblings, 0 replies; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-04 20:42 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, 4 Mar 2025 at 11:35, Álvaro Herrera <[email protected]> wrote:
> https://commitfest.postgresql.org/me
I've restored the original homepage and moved this new dashboard
(minus the "Archive" link) to /me:
https://commitfest-test.postgresql.org/me/
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 20:50 Robert Haas <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
4 siblings, 0 replies; 30+ messages in thread
From: Robert Haas @ 2025-03-04 20:50 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Mon, Mar 3, 2025 at 8:21 PM Jelte Fennema-Nio <[email protected]> wrote:
> As always, please test out the current staging website[1] to give some feedback.
> HTTP auth user and password are both pgtest.
So, that worked for me, but then it wants me to log into my
postgreql.org account, and that doesn't seem to work.
> Also I wanted to highlight the work Jacob Brazeal is doing again. He
> has been working on an AI-powered summarization and patch review
> recommendation engine[2]. I definitely recommend people to take a look
> at that and leave some feedback.
Thanks for highlighting this.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 21:01 Andreas Karlsson <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Andreas Karlsson @ 2025-03-04 21:01 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; Amit Kapila <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On 3/4/25 2:30 PM, Jelte Fennema-Nio wrote:
> On Tue, 4 Mar 2025 at 13:36, Amit Kapila <[email protected]> wrote:
>>
>> On Tue, Mar 4, 2025 at 4:05 PM Álvaro Herrera <[email protected]> wrote:
>>> I think showing different pages on the same URL depending on whether
>>> you're logged in or not is not great UX.
>>>
>>
>> +1. The default should be what we see today, and there should be some
>> way to see the patches in which a particular person is involved.
>
> I'm quite surprised that people seem to love the content of the
> current homepage so much. Could someone explain why they want to see
> this full list of commitfests as the first page you see? I feel like
> I'm missing something here.
What I need to see is the below (plus any future commit fests).
2025-07 (Open - 2025-07-01 - 2025-07-31)
2025-03 (In Progress - 2025-03-01 - 2025-03-31)
2025-01 (Closed - 2025-01-01 - 2025-01-31)
I am interested in the dates when commit fests open and close and to be
able to quickly navigate to the open, the in progress and the latest
closed one.
It can obviously be displayed in many other ways but the current way is
pretty convenient.
Andreas
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-04 23:14 Jelte Fennema-Nio <[email protected]>
parent: Andreas Karlsson <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-04 23:14 UTC (permalink / raw)
To: Andreas Karlsson <[email protected]>; +Cc: Amit Kapila <[email protected]>; Álvaro Herrera <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Tue, 4 Mar 2025 at 22:01, Andreas Karlsson <[email protected]> wrote:
> What I need to see is the below (plus any future commit fests).
Thanks you for describing how you use the current homepage. That's
super helpful.
> I am interested in the dates when commit fests open and close
These are the same exact 5 months every year. Maybe the homepage
should contain some basic, mostly static, info at the top like:
There are 5 commitfests for each PostgreSQL release. They take place
in the months July (the first of the development cycle), September,
November, January and March (the last before the feature freeze). A
commitfest lasts for the full month. Patches can be submitted to a
commitfest before that month has started. After a commitfest has
started new patches need to be registered for the following
commitfest. The .../No commitfest is currently in progress. The next
commitfest starts on ...
> and to be
> able to quickly navigate to the open, the in progress and the latest
> closed one.
Could you elaborate why you want to navigate to the latest closed one?
Is that to move your own patches (or ones that you are reviewing) over
to the next one? If so it sounds like that would not be necessary
anymore with the new dashboard page. If you have some other reason I'm
very curious to know.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-05 00:24 Andreas Karlsson <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Andreas Karlsson @ 2025-03-05 00:24 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: Amit Kapila <[email protected]>; Álvaro Herrera <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On 3/5/25 12:14 AM, Jelte Fennema-Nio wrote:
> On Tue, 4 Mar 2025 at 22:01, Andreas Karlsson <[email protected]> wrote:
>> What I need to see is the below (plus any future commit fests).
>
> Thanks you for describing how you use the current homepage. That's
> super helpful.
>
>> I am interested in the dates when commit fests open and close
>
> These are the same exact 5 months every year. Maybe the homepage
> should contain some basic, mostly static, info at the top like:
>
> There are 5 commitfests for each PostgreSQL release. They take place
> in the months July (the first of the development cycle), September,
> November, January and March (the last before the feature freeze). A
> commitfest lasts for the full month. Patches can be submitted to a
> commitfest before that month has started. After a commitfest has
> started new patches need to be registered for the following
> commitfest. The .../No commitfest is currently in progress. The next
> commitfest starts on ...
Yup, and I and other (in my case formerly; since this January)
non-professional PostgreSQL developers have no chance to remember when
the commit fests are. And I suspect some of the professionals don't
always remember. :) And this is data I have often wanted.
>> and to be
>> able to quickly navigate to the open, the in progress and the latest
>> closed one.
>
> Could you elaborate why you want to navigate to the latest closed one?
> Is that to move your own patches (or ones that you are reviewing) over
> to the next one? If so it sounds like that would not be necessary
> anymore with the new dashboard page. If you have some other reason I'm
> very curious to know.
Yes, to move patches over to the next commit fest.
Andreas
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-06 16:11 Peter Eisentraut <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Peter Eisentraut @ 2025-03-06 16:11 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On 04.03.25 21:37, Jelte Fennema-Nio wrote:
> 1. This new homepage includes open patches from*all* commitfests. And
> there's currently no page with that information.
Ok, that's interesting, but I'm even less sure why that should be the
default view. The whole point of chunking things into commitfests is to
have a focused view on what to do now and what to do later.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-06 17:08 Tom Lane <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Tom Lane @ 2025-03-06 17:08 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
Peter Eisentraut <[email protected]> writes:
> On 04.03.25 21:37, Jelte Fennema-Nio wrote:
>> 1. This new homepage includes open patches from*all* commitfests. And
>> there's currently no page with that information.
> Ok, that's interesting, but I'm even less sure why that should be the
> default view. The whole point of chunking things into commitfests is to
> have a focused view on what to do now and what to do later.
Indeed, that choice seems completely astonishing. You just kicked to
the curb all of the discussion at FOSDEM about how patches shouldn't
be carried forward automatically. Effectively this means patches
will *never* go away until somebody closes them explicitly.
regards, tom lane
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-06 17:27 Jelte Fennema-Nio <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-06 17:27 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>; Jacob Brazeal <[email protected]>
On Thu, 6 Mar 2025 at 18:08, Tom Lane <[email protected]> wrote:
>
> Peter Eisentraut <[email protected]> writes:
> > On 04.03.25 21:37, Jelte Fennema-Nio wrote:
> >> 1. This new homepage includes open patches from*all* commitfests. And
> >> there's currently no page with that information.
>
> > Ok, that's interesting, but I'm even less sure why that should be the
> > default view. The whole point of chunking things into commitfests is to
> > have a focused view on what to do now and what to do later.
>
> Indeed, that choice seems completely astonishing. You just kicked to
> the curb all of the discussion at FOSDEM about how patches shouldn't
> be carried forward automatically.
I see where you're coming from, but I think you're exaggerating a bit.
The commitfest that each patch is part of is visible in the CF column
on the new page. Patches from old commitfests color-coded red, and are
sorted below new patches from future commitfests, which are again
sorted below patches of the active commitfest. Your own old patches
that you are supposed to move/close are shown at the top of your page,
to remind authors to actually do that.
To be clear: This new view does not replace the homepage anymore, it's
now a separate /me page.
> Effectively this means patches
> will *never* go away until somebody closes them explicitly.
True, but it will also cause authors to actually close/move those
patches explicitly. And if an author doesn't close/move their patch in
an old commitfest for some time, it seems completely reasonable for
someone else to close that patch as Withdrawn/Rejected (or maybe a new
status like Inactive). I'd say let's try this new view out like this
and see what problems people actually have when using it. Especially
since this is in addition to the existing views, not replacing
anything.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-17 22:11 Jelte Fennema-Nio <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
4 siblings, 1 reply; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-17 22:11 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>
On Tue, 4 Mar 2025 at 02:21, Jelte Fennema-Nio <[email protected]> wrote:
>
> The next commitfest app release is planned for March 18th
I deployed the latest release of the commitfest app. Below is a
changelog that's slightly updated.
1. Major change: There's a new /me page which shows a dashboard of
open patches where you are author/reviewer/committer. These patches
are ordered & grouped in a hopefully useful way. Peter Geoghegan
suggested adding a "dashboard" of this kind. It's the first attempt
and can probably use some fine tuning based on feedback. So please try
it out and let me know what you think. I almost certainly won't have
time to make big changes to it in the next ~month though.
2. Show name of a committer in the "Committer" column (instead of only
the username).
3. Fix the "Review" form so that all checkboxes can actually be
clicked. Thanks to Maciek.
4. Allow sorting patches by "failing since", this can be done by
clicking the header. This will take a few days to work correctly,
since a "failing since" timestamp was not being collected before.
5. Remove the "latest activity" column. This did not contain useful
information, that "latest email" didn't already provide.
6. The "latest email" column now shows "time since" (e.g. 1 week ago)
by default instead of an exact timestamp. You can still see the exact
timestamp by hovering over the cell. If you prefer to see the exact
timestamp only, you can change this as a setting using the "edit
profile" link in the top right corner.
7. Searching patches by author/reviewer now isn't a dropdown with a
ton of options, but instead has become a dropdown with a search box.
This also greatly improves page load performance: By not putting all
users in the HTML as a dropdown option it's saving 600-700ms in my
testing.
8. Bugfix: Correctly show CI timeout as failure.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-21 16:41 Peter Eisentraut <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Peter Eisentraut @ 2025-03-21 16:41 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>
On 17.03.25 23:11, Jelte Fennema-Nio wrote:
> 1. Major change: There's a new /me page which shows a dashboard of
> open patches where you are author/reviewer/committer. These patches
> are ordered & grouped in a hopefully useful way. Peter Geoghegan
> suggested adding a "dashboard" of this kind. It's the first attempt
> and can probably use some fine tuning based on feedback.
Thanks, this is very promising.
I have a few pieces of feedback that could be addressed by additional
rules for how things are listed in the dashboard:
- If I'm the committer for a patch but not a reviewer, and the patch is
in "needs review" status, then the patch is formally speaking not
actionable by me and should not be under "Patches that are ready for
your review". Perhaps it should be under "Blocked on others" [namely
the reviewers], or in a different category.
- Conversely, if I'm the reviewer for a patch but not the committer, and
the patch is in "ready for committer" status, then it's also not
"Patches that are ready for your review". This might similarly be
"Blocked on others" [namely the committer].
- Also, my dashboard shows patches from past and future commitfests. I
don't know what I'm supposed to do with that. The purpose of having a
"current" commitfest is that you work on that one when it's current.
The patches in past and future commitfests were presumably put there for
reasons that mean that they are not currently to be worked on as a priority.
(I'm probably not representative, but my current dashboard has a
somewhat low hit ratio because of the above points.)
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-21 17:53 Tom Lane <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Tom Lane @ 2025-03-21 17:53 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>
Peter Eisentraut <[email protected]> writes:
> I have a few pieces of feedback that could be addressed by additional
> rules for how things are listed in the dashboard:
> - If I'm the committer for a patch but not a reviewer, and the patch is
> in "needs review" status, then the patch is formally speaking not
> actionable by me and should not be under "Patches that are ready for
> your review". Perhaps it should be under "Blocked on others" [namely
> the reviewers], or in a different category.
> - Conversely, if I'm the reviewer for a patch but not the committer, and
> the patch is in "ready for committer" status, then it's also not
> "Patches that are ready for your review". This might similarly be
> "Blocked on others" [namely the committer].
Both of these things would work correctly only for patches that you
have already claimed as committer. I'm not sure about other people,
but I rarely claim a patch as committer until I'm actually on the
verge of committing it. I might mark myself as reviewer sometime
sooner than that, in which case your second proposal would be a
net negative for me. I don't think we should encourage committers
to claim patches early, because then they are a single point of
failure (work-stoppage) in a way that a reviewer is not. Maybe
we need a way for committers to mark patches as things they want
to pay attention to, without thereby blocking other committers
from taking up the patch?
> - Also, my dashboard shows patches from past and future commitfests. I
> don't know what I'm supposed to do with that. The purpose of having a
> "current" commitfest is that you work on that one when it's current.
This might make sense for the patch author, but I agree it's not
appropriate for anyone else.
regards, tom lane
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-22 11:15 Jelte Fennema-Nio <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 2 replies; 30+ messages in thread
From: Jelte Fennema-Nio @ 2025-03-22 11:15 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>
On Fri, 21 Mar 2025 at 18:53, Tom Lane <[email protected]> wrote:
>
> Peter Eisentraut <[email protected]> writes:
> > - If I'm the committer for a patch but not a reviewer, and the patch is
> > in "needs review" status, then the patch is formally speaking not
> > actionable by me and should not be under "Patches that are ready for
> > your review". Perhaps it should be under "Blocked on others" [namely
> > the reviewers], or in a different category.
I think I agree with this... but is that actually a common situation?
I'd expect people to mostly "claim a patch as committer" when it's in
the "ready for committer" state.
> > - Conversely, if I'm the reviewer for a patch but not the committer, and
> > the patch is in "ready for committer" status, then it's also not
> > "Patches that are ready for your review". This might similarly be
> > "Blocked on others" [namely the committer].
>
> Both of these things would work correctly only for patches that you
> have already claimed as committer. I'm not sure about other people,
> but I rarely claim a patch as committer until I'm actually on the
> verge of committing it. I might mark myself as reviewer sometime
> sooner than that, in which case your second proposal would be a
> net negative for me.
I think probably a nice intermediary solution would be to handle
patches with an assigned committer differently. If a committer
assigned themselves as committer and it's "ready for committer", then
I don't think it should be marked as "ready for your review" for other
committers. If there's no committer, I think it should be, because
*some committer* needs to review the patch but it's unknown which one.
> I don't think we should encourage committers
> to claim patches early, because then they are a single point of
> failure (work-stoppage) in a way that a reviewer is not. Maybe
> we need a way for committers to mark patches as things they want
> to pay attention to, without thereby blocking other committers
> from taking up the patch?
Yeah I think we need a way to "star" a patch (not just for committers,
but for anyone). After creating this initial version of this dashboard
I realized it's also possible to "subscribe to updates" for a patch,
which means you'll get emails about it. I think if you "subscribe for
updates" you should also see it in your dashboard (which you currently
don't). But I also think there should be a way to "star" something, so
you see it in your dashboard without receiving email updates about it.
> > - Also, my dashboard shows patches from past and future commitfests. I
> > don't know what I'm supposed to do with that. The purpose of having a
> > "current" commitfest is that you work on that one when it's current.
>
> This might make sense for the patch author, but I agree it's not
> appropriate for anyone else.
I'd love to hear some more thoughts on this. Especially from Peter G,
because he explicitly requested to see patches from different
commitfests in this dashboard. So I'm curious about his reasoning.
Ofcourse we can make this configurable or have two separate pages, but
I'm wondering if there's a way to change it in a way that works for
everyone.
Instead of showing all patches from all commitfests, how about we do
the following:
1. Show all open patches where you are the author, no matter which
commitfest they are in.
2. If there's currently no commitfest "In Progress", then we show all
patches (where you are not the author) both from the Previous
commitfest and the Open commitfest. I think it would be a shame if
everyone's dashboard is empty in the first week after a commitfest,
just because authors haven't moved their patches over to the Open
commitfest.
3. If there's a commitfest "In Progress", then we only show patches
(where you are not the author) from the "In Progress" commitfest to
keep reviews focussed.
4. Have a separate section at the bottom of the page with all the
patches that you are tracking, but are not matching one of the
criteria from the three rules above.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-22 14:31 Peter Geoghegan <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
1 sibling, 1 reply; 30+ messages in thread
From: Peter Geoghegan @ 2025-03-22 14:31 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Maciek Sakrejda <[email protected]>
On Sat, Mar 22, 2025 at 7:15 AM Jelte Fennema-Nio <[email protected]> wrote:
> Yeah I think we need a way to "star" a patch (not just for committers,
> but for anyone). After creating this initial version of this dashboard
> I realized it's also possible to "subscribe to updates" for a patch,
> which means you'll get emails about it. I think if you "subscribe for
> updates" you should also see it in your dashboard (which you currently
> don't). But I also think there should be a way to "star" something, so
> you see it in your dashboard without receiving email updates about it.
Right. In my view "starring" something should definitely not appear in
the "History Log" of the patch; it should be private.
> > > - Also, my dashboard shows patches from past and future commitfests. I
> > > don't know what I'm supposed to do with that. The purpose of having a
> > > "current" commitfest is that you work on that one when it's current.
> >
> > This might make sense for the patch author, but I agree it's not
> > appropriate for anyone else.
>
> I'd love to hear some more thoughts on this. Especially from Peter G,
> because he explicitly requested to see patches from different
> commitfests in this dashboard. So I'm curious about his reasoning.
> Ofcourse we can make this configurable or have two separate pages, but
> I'm wondering if there's a way to change it in a way that works for
> everyone.
Personally, I think it'd be most useful for the dashboard to show all
open patches. I believe that that implies that it'll only show me
patches from either the current CF, or the next CF. But thinking about
it some more...I guess that there are edge-cases, where patches can be
in limbo between being "fully open" and "fully closed". Like when a
patch from an old CF isn't brought forward, and also isn't
withdrawn/returned.
I think that we should apply an expansive definition of "active", that
still shows me things that are less than 100% officially closed.
Possibly with additional visual cues that they're in this in
between/limbo state.
> Instead of showing all patches from all commitfests, how about we do
> the following:
I think that we should show all patches from all commitfests in the
"Personal Dashboard", provided that they're not closed. I probably
don't have enough experience with the current "Personal Dashboard"
yet, but so far it seems like exactly what I had in mind.
If I see something in the "Personal Dashboard" that seems like it
shouldn't be there, due to not being in either the current or next CF,
and am bothered by that, then maybe I should then take it as an
opportunity to fix the problem. As I said, maybe there should be some
additional visual cue that shows certain "Personal Dashboard" entries
are "in limbo". But please don't make entries in this state just
vanish, on the grounds that they're theoretically closed -- experience
suggests that they're probably not really closed at all.
If something appears on my "Personal Dashboard" it appears there
because I actively chose to participate, and hiding things from me on
bureaucratic grounds seems paternalistic.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-22 14:49 Tom Lane <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Tom Lane @ 2025-03-22 14:49 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Maciek Sakrejda <[email protected]>
Peter Geoghegan <[email protected]> writes:
> On Sat, Mar 22, 2025 at 7:15 AM Jelte Fennema-Nio <[email protected]> wrote:
>> Yeah I think we need a way to "star" a patch (not just for committers,
>> but for anyone).
> Right. In my view "starring" something should definitely not appear in
> the "History Log" of the patch; it should be private.
+1
> Personally, I think it'd be most useful for the dashboard to show all
> open patches.
Surely not "all"? We have that display already. Should be more like
"patches that I have expressed an interest in or have reason to take
current or future action on".
In the case of stale patches that haven't been moved forward from a
closed commitfest, perhaps we could compromise on Jelte's suggestion
of putting those in a separate section at the bottom of the page.
However, I still think such patches should be treated differently for
the author than other people. The author does have current action to
take on the patch, namely moving it forward or withdrawing it.
regards, tom lane
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-22 15:05 Peter Geoghegan <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Peter Geoghegan @ 2025-03-22 15:05 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Maciek Sakrejda <[email protected]>
On Sat, Mar 22, 2025 at 10:49 AM Tom Lane <[email protected]> wrote:
> > Personally, I think it'd be most useful for the dashboard to show all
> > open patches.
>
> Surely not "all"? We have that display already. Should be more like
> "patches that I have expressed an interest in or have reason to take
> current or future action on".
What I meant was "patches that I have expressed an interest in or have
reason to take current or future action on" should be interpreted in
the broadest/most permissive way possible by the dashboard. In
particular, entries should only *fully* disappear when somebody (some
individual human) has actively chosen to make a representation that
the patch is closed out. The rest (how we present that information) is
details.
> In the case of stale patches that haven't been moved forward from a
> closed commitfest, perhaps we could compromise on Jelte's suggestion
> of putting those in a separate section at the bottom of the page.
If there is any gray area, then I very much want us to err on the side
of still showing *something*. I really strongly object to having
things vanish, absent a 100% unambiguous signal that that's what
should happen.
In general, the personal dashboard is (quite usefully) oriented around
what actions you as a user (or some other CF app user) needs to take
-- it is workflow oriented. It seems natural to me to handle stale/in
limbo patches (patches that are not officially closed but also aren't
in the current or next CF) in just the same way -- by presenting the
information in terms of actions that need to be taken by some
individual stakeholder.
> However, I still think such patches should be treated differently for
> the author than other people. The author does have current action to
> take on the patch, namely moving it forward or withdrawing it.
Right. So maybe for the patch author it appears either under "Your
patches that need changes from you", or in a similar section that's
just for stale patches that need to either be officially dropped or
officially moved forward to an open CF. Whereas it'd be a little
different (but not too different) for somebody who sees a patch
because they're the reviewer/committer of record (the action item for
such a person, if any, is to actually fully close the patch, or to nag
the patch author).
It probably makes sense to make stale/in limbo entries stick out like
a sore thumb. They're *supposed* to be annoying.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Next commitfest app release is planned for March 18th
@ 2025-03-24 09:55 Peter Eisentraut <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
1 sibling, 0 replies; 30+ messages in thread
From: Peter Eisentraut @ 2025-03-24 09:55 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Peter Geoghegan <[email protected]>; Maciek Sakrejda <[email protected]>
On 22.03.25 12:15, Jelte Fennema-Nio wrote:
> On Fri, 21 Mar 2025 at 18:53, Tom Lane <[email protected]> wrote:
>>
>> Peter Eisentraut <[email protected]> writes:
>>> - If I'm the committer for a patch but not a reviewer, and the patch is
>>> in "needs review" status, then the patch is formally speaking not
>>> actionable by me and should not be under "Patches that are ready for
>>> your review". Perhaps it should be under "Blocked on others" [namely
>>> the reviewers], or in a different category.
>
> I think I agree with this... but is that actually a common situation?
> I'd expect people to mostly "claim a patch as committer" when it's in
> the "ready for committer" state.
It's probably not common, but I seem to recall that at the FOSDEM
developer meeting it was explicitly suggested that committers sign on to
patches earlier so that authors and reviewers have a better idea of whom
to target their patches for. Which is why I started doing that now.
> Yeah I think we need a way to "star" a patch (not just for committers,
> but for anyone). After creating this initial version of this dashboard
> I realized it's also possible to "subscribe to updates" for a patch,
> which means you'll get emails about it. I think if you "subscribe for
> updates" you should also see it in your dashboard (which you currently
> don't). But I also think there should be a way to "star" something, so
> you see it in your dashboard without receiving email updates about it.
It would be good to able to see subscribed-to patches on the dashboard
as well. But let's not create two slightly different mechanisms for this.
>>> - Also, my dashboard shows patches from past and future commitfests. I
>>> don't know what I'm supposed to do with that. The purpose of having a
>>> "current" commitfest is that you work on that one when it's current.
>>
>> This might make sense for the patch author, but I agree it's not
>> appropriate for anyone else.
I don't mind seeing them, just not under "patches that are ready for
your review". If there were other subheadings like "future patches",
"patches forgotten in the past" etc. it would be better.
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH 1/5] Add REPACK command
@ 2026-02-16 19:49 Antonin Houska <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Antonin Houska @ 2026-02-16 19:49 UTC (permalink / raw)
REPACK absorbs the functionality of VACUUM FULL and CLUSTER in a single
command. Because this functionality is completely different from
regular VACUUM, having it separate from VACUUM makes it easier for users
to understand; as for CLUSTER, the term is heavily overloaded in the
IT world and even in Postgres itself, so it's good that we can avoid it.
Author: Antonin Houska <[email protected]>
Co-authored-by: =C3=81lvaro Herrera <[email protected]>
Reviewed-by: Mihail Nikalayeu <[email protected]>
Reviewed-by: Robert Treat <[email protected]>
Reviewed-by: Euler Taveira <[email protected]>
Reviewed-by: Matheus Alcantara <[email protected]>
Reviewed-by: Junwang Zhao <[email protected]>
Reviewed-by: jian he <[email protected]>
Discussion: https://postgr.es/m/82651.1720540558@antos
Discussion: https://postgr.es/m/[email protected]
---
doc/src/sgml/monitoring.sgml | 223 +++++-
doc/src/sgml/ref/allfiles.sgml | 1 +
doc/src/sgml/ref/cluster.sgml | 97 +--
doc/src/sgml/ref/repack.sgml | 328 +++++++++
doc/src/sgml/ref/vacuum.sgml | 33 +-
doc/src/sgml/reference.sgml | 1 +
src/backend/access/heap/heapam_handler.c | 32 +-
src/backend/catalog/index.c | 2 +-
src/backend/catalog/system_views.sql | 29 +-
src/backend/commands/cluster.c | 842 +++++++++++++++--------
src/backend/commands/vacuum.c | 6 +-
src/backend/parser/gram.y | 86 ++-
src/backend/tcop/utility.c | 23 +-
src/backend/utils/adt/pgstatfuncs.c | 4 +-
src/bin/psql/tab-complete.in.c | 42 +-
src/include/commands/cluster.h | 8 +-
src/include/commands/progress.h | 50 +-
src/include/nodes/parsenodes.h | 35 +-
src/include/parser/kwlist.h | 1 +
src/include/tcop/cmdtaglist.h | 1 +
src/include/utils/backend_progress.h | 2 +-
src/test/regress/expected/cluster.out | 134 +++-
src/test/regress/expected/rules.out | 72 +-
src/test/regress/sql/cluster.sql | 70 +-
src/tools/pgindent/typedefs.list | 2 +
25 files changed, 1596 insertions(+), 528 deletions(-)
create mode 100644 doc/src/sgml/ref/repack.sgml
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index b77d189a500..71c92ed53ef 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -405,6 +405,14 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss =
11:34 0:00 postgres: ser
</entry>
</row>
=20
+ <row>
+ <entry><structname>pg_stat_progress_repack</structname><indexterm><p=
rimary>pg_stat_progress_repack</primary></indexterm></entry>
+ <entry>One row for each backend running
+ <command>REPACK</command>, showing current progress. See
+ <xref linkend=3D"repack-progress-reporting"/>.
+ </entry>
+ </row>
+
<row>
<entry><structname>pg_stat_progress_basebackup</structname><indexter=
m><primary>pg_stat_progress_basebackup</primary></indexterm></entry>
<entry>One row for each WAL sender process streaming a base backup,
@@ -5646,7 +5654,8 @@ FROM pg_stat_get_backend_idset() AS backendid;
certain commands during command execution. Currently, the only commands
which support progress reporting are <command>ANALYZE</command>,
<command>CLUSTER</command>,
- <command>CREATE INDEX</command>, <command>VACUUM</command>,
+ <command>CREATE INDEX</command>, <command>REPACK</command>,
+ <command>VACUUM</command>,
<command>COPY</command>,
and <xref linkend=3D"protocol-replication-base-backup"/> (i.e., replica=
tion
command that <xref linkend=3D"app-pgbasebackup"/> issues to take
@@ -6130,6 +6139,218 @@ FROM pg_stat_get_backend_idset() AS backendid;
</table>
</sect2>
=20
+ <sect2 id=3D"repack-progress-reporting">
+ <title>REPACK Progress Reporting</title>
+
+ <indexterm>
+ <primary>pg_stat_progress_repack</primary>
+ </indexterm>
+
+ <para>
+ Whenever <command>REPACK</command> is running,
+ the <structname>pg_stat_progress_repack</structname> view will contain a
+ row for each backend that is currently running the command. The tables
+ below describe the information that will be reported and provide
+ information about how to interpret it.
+ </para>
+
+ <table id=3D"pg-stat-progress-repack-view" xreflabel=3D"pg_stat_progress=
_repack">
+ <title><structname>pg_stat_progress_repack</structname> View</title>
+ <tgroup cols=3D"1">
+ <thead>
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ Column Type
+ </para>
+ <para>
+ Description
+ </para></entry>
+ </row>
+ </thead>
+
+ <tbody>
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>pid</structfield> <type>integer</type>
+ </para>
+ <para>
+ Process ID of backend.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>datid</structfield> <type>oid</type>
+ </para>
+ <para>
+ OID of the database to which this backend is connected.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>datname</structfield> <type>name</type>
+ </para>
+ <para>
+ Name of the database to which this backend is connected.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>relid</structfield> <type>oid</type>
+ </para>
+ <para>
+ OID of the table being repacked.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>phase</structfield> <type>text</type>
+ </para>
+ <para>
+ Current processing phase. See <xref linkend=3D"repack-phases"/>.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>repack_index_relid</structfield> <type>oid</type>
+ </para>
+ <para>
+ If the table is being scanned using an index, this is the OID of the
+ index being used; otherwise, it is zero.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>heap_tuples_scanned</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of heap tuples scanned.
+ This counter only advances when the phase is
+ <literal>seq scanning heap</literal>,
+ <literal>index scanning heap</literal>
+ or <literal>writing new heap</literal>.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>heap_tuples_written</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of heap tuples written.
+ This counter only advances when the phase is
+ <literal>seq scanning heap</literal>,
+ <literal>index scanning heap</literal>
+ or <literal>writing new heap</literal>.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>heap_blks_total</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Total number of heap blocks in the table. This number is reported
+ as of the beginning of <literal>seq scanning heap</literal>.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>heap_blks_scanned</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of heap blocks scanned. This counter only advances when the
+ phase is <literal>seq scanning heap</literal>.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role=3D"catalog_table_entry"><para role=3D"column_definition">
+ <structfield>index_rebuild_count</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of indexes rebuilt. This counter only advances when the pha=
se
+ is <literal>rebuilding index</literal>.
+ </para></entry>
+ </row>
+ </tbody>
+ </tgroup>
+ </table>
+
+ <table id=3D"repack-phases">
+ <title>REPACK Phases</title>
+ <tgroup cols=3D"2">
+ <colspec colname=3D"col1" colwidth=3D"1*"/>
+ <colspec colname=3D"col2" colwidth=3D"2*"/>
+ <thead>
+ <row>
+ <entry>Phase</entry>
+ <entry>Description</entry>
+ </row>
+ </thead>
+
+ <tbody>
+ <row>
+ <entry><literal>initializing</literal></entry>
+ <entry>
+ The command is preparing to begin scanning the heap. This phase is
+ expected to be very brief.
+ </entry>
+ </row>
+ <row>
+ <entry><literal>seq scanning heap</literal></entry>
+ <entry>
+ The command is currently scanning the table using a sequential scan.
+ </entry>
+ </row>
+ <row>
+ <entry><literal>index scanning heap</literal></entry>
+ <entry>
+ <command>REPACK</command> is currently scanning the table using an =
index scan.
+ </entry>
+ </row>
+ <row>
+ <entry><literal>sorting tuples</literal></entry>
+ <entry>
+ <command>REPACK</command> is currently sorting tuples.
+ </entry>
+ </row>
+ <row>
+ <entry><literal>writing new heap</literal></entry>
+ <entry>
+ <command>REPACK</command> is currently writing the new heap.
+ </entry>
+ </row>
+ <row>
+ <entry><literal>swapping relation files</literal></entry>
+ <entry>
+ The command is currently swapping newly-built files into place.
+ </entry>
+ </row>
+ <row>
+ <entry><literal>rebuilding index</literal></entry>
+ <entry>
+ The command is currently rebuilding an index.
+ </entry>
+ </row>
+ <row>
+ <entry><literal>performing final cleanup</literal></entry>
+ <entry>
+ The command is performing final cleanup. When this phase is
+ completed, <command>REPACK</command> will end.
+ </entry>
+ </row>
+ </tbody>
+ </tgroup>
+ </table>
+ </sect2>
+
<sect2 id=3D"copy-progress-reporting">
<title>COPY Progress Reporting</title>
=20
diff --git a/doc/src/sgml/ref/allfiles.sgml b/doc/src/sgml/ref/allfiles.sgml
index e167406c744..141ada9c50a 100644
--- a/doc/src/sgml/ref/allfiles.sgml
+++ b/doc/src/sgml/ref/allfiles.sgml
@@ -167,6 +167,7 @@ Complete list of usable sgml source files in this direc=
tory.
<!ENTITY refreshMaterializedView SYSTEM "refresh_materialized_view.sgml">
<!ENTITY reindex SYSTEM "reindex.sgml">
<!ENTITY releaseSavepoint SYSTEM "release_savepoint.sgml">
+<!ENTITY repack SYSTEM "repack.sgml">
<!ENTITY reset SYSTEM "reset.sgml">
<!ENTITY revoke SYSTEM "revoke.sgml">
<!ENTITY rollback SYSTEM "rollback.sgml">
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 0b47460080b..2cda711bc9f 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -33,51 +33,13 @@ CLUSTER [ ( <replaceable class=3D"parameter">option</re=
placeable> [, ...] ) ] [ <r
<title>Description</title>
=20
<para>
- <command>CLUSTER</command> instructs <productname>PostgreSQL</productna=
me>
- to cluster the table specified
- by <replaceable class=3D"parameter">table_name</replaceable>
- based on the index specified by
- <replaceable class=3D"parameter">index_name</replaceable>. The index mu=
st
- already have been defined on
- <replaceable class=3D"parameter">table_name</replaceable>.
+ The <command>CLUSTER</command> command is equivalent to
+ <xref linkend=3D"sql-repack"/> with an <literal>USING INDEX</literal>
+ clause. See there for more details.
</para>
=20
- <para>
- When a table is clustered, it is physically reordered
- based on the index information. Clustering is a one-time operation:
- when the table is subsequently updated, the changes are
- not clustered. That is, no attempt is made to store new or
- updated rows according to their index order. (If one wishes, one can
- periodically recluster by issuing the command again. Also, setting
- the table's <literal>fillfactor</literal> storage parameter to less than
- 100% can aid in preserving cluster ordering during updates, since updat=
ed
- rows are kept on the same page if enough space is available there.)
- </para>
-
- <para>
- When a table is clustered, <productname>PostgreSQL</productname>
- remembers which index it was clustered by. The form
- <command>CLUSTER <replaceable class=3D"parameter">table_name</replaceab=
le></command>
- reclusters the table using the same index as before. You can also
- use the <literal>CLUSTER</literal> or <literal>SET WITHOUT CLUSTER</lit=
eral>
- forms of <link linkend=3D"sql-altertable"><command>ALTER TABLE</command=
></link> to set the index to be used for
- future cluster operations, or to clear any previous setting.
- </para>
-
- <para>
- <command>CLUSTER</command> without a
- <replaceable class=3D"parameter">table_name</replaceable> reclusters al=
l the
- previously-clustered tables in the current database that the calling us=
er
- has privileges for. This form of <command>CLUSTER</command> cannot be
- executed inside a transaction block.
- </para>
+<!-- Do we need to describe exactly which options map to what? They seem =
obvious to me. -->
=20
- <para>
- When a table is being clustered, an <literal>ACCESS
- EXCLUSIVE</literal> lock is acquired on it. This prevents any other
- database operations (both reads and writes) from operating on the
- table until the <command>CLUSTER</command> is finished.
- </para>
</refsect1>
=20
<refsect1>
@@ -136,63 +98,12 @@ CLUSTER [ ( <replaceable class=3D"parameter">option</r=
eplaceable> [, ...] ) ] [ <r
on the table.
</para>
=20
- <para>
- In cases where you are accessing single rows randomly
- within a table, the actual order of the data in the
- table is unimportant. However, if you tend to access some
- data more than others, and there is an index that groups
- them together, you will benefit from using <command>CLUSTER</command>.
- If you are requesting a range of indexed values from a table, or a
- single indexed value that has multiple rows that match,
- <command>CLUSTER</command> will help because once the index identifies=
the
- table page for the first row that matches, all other rows
- that match are probably already on the same table page,
- and so you save disk accesses and speed up the query.
- </para>
-
- <para>
- <command>CLUSTER</command> can re-sort the table using either an index=
scan
- on the specified index, or (if the index is a b-tree) a sequential
- scan followed by sorting. It will attempt to choose the method that
- will be faster, based on planner cost parameters and available statist=
ical
- information.
- </para>
-
<para>
While <command>CLUSTER</command> is running, the <xref
linkend=3D"guc-search-path"/> is temporarily changed to <literal>pg_ca=
talog,
pg_temp</literal>.
</para>
=20
- <para>
- When an index scan is used, a temporary copy of the table is created t=
hat
- contains the table data in the index order. Temporary copies of each
- index on the table are created as well. Therefore, you need free spac=
e on
- disk at least equal to the sum of the table size and the index sizes.
- </para>
-
- <para>
- When a sequential scan and sort is used, a temporary sort file is
- also created, so that the peak temporary space requirement is as much
- as double the table size, plus the index sizes. This method is often
- faster than the index scan method, but if the disk space requirement is
- intolerable, you can disable this choice by temporarily setting <xref
- linkend=3D"guc-enable-sort"/> to <literal>off</literal>.
- </para>
-
- <para>
- It is advisable to set <xref linkend=3D"guc-maintenance-work-mem"/> to
- a reasonably large value (but not more than the amount of RAM you can
- dedicate to the <command>CLUSTER</command> operation) before clusterin=
g.
- </para>
-
- <para>
- Because the planner records statistics about the ordering of
- tables, it is advisable to run <link linkend=3D"sql-analyze"><command>=
ANALYZE</command></link>
- on the newly clustered table.
- Otherwise, the planner might make poor choices of query plans.
- </para>
-
<para>
Because <command>CLUSTER</command> remembers which indexes are cluster=
ed,
one can cluster the tables one wants clustered manually the first time,
diff --git a/doc/src/sgml/ref/repack.sgml b/doc/src/sgml/ref/repack.sgml
new file mode 100644
index 00000000000..61d5c2cdef1
--- /dev/null
+++ b/doc/src/sgml/ref/repack.sgml
@@ -0,0 +1,328 @@
+<!--
+doc/src/sgml/ref/repack.sgml
+PostgreSQL documentation
+-->
+
+<refentry id=3D"sql-repack">
+ <indexterm zone=3D"sql-repack">
+ <primary>REPACK</primary>
+ </indexterm>
+
+ <refmeta>
+ <refentrytitle>REPACK</refentrytitle>
+ <manvolnum>7</manvolnum>
+ <refmiscinfo>SQL - Language Statements</refmiscinfo>
+ </refmeta>
+
+ <refnamediv>
+ <refname>REPACK</refname>
+ <refpurpose>rewrite a table to reclaim disk space</refpurpose>
+ </refnamediv>
+
+ <refsynopsisdiv>
+<synopsis>
+REPACK [ ( <replaceable class=3D"parameter">option</replaceable> [, ...] )=
] [ <replaceable class=3D"parameter">table_and_columns</replaceable> [ USI=
NG INDEX [ <replaceable class=3D"parameter">index_name</replaceable> ] ] ]
+REPACK [ ( <replaceable class=3D"parameter">option</replaceable> [, ...] )=
] USING INDEX
+
+<phrase>where <replaceable class=3D"parameter">option</replaceable> can be=
one of:</phrase>
+
+ VERBOSE [ <replaceable class=3D"parameter">boolean</replaceable> ]
+ ANALYZE [ <replaceable class=3D"parameter">boolean</replaceable> ]
+
+<phrase>and <replaceable class=3D"parameter">table_and_columns</replaceabl=
e> is:</phrase>
+
+ <replaceable class=3D"parameter">table_name</replaceable> [ ( <replace=
able class=3D"parameter">column_name</replaceable> [, ...] ) ]
+</synopsis>
+ </refsynopsisdiv>
+
+ <refsect1>
+ <title>Description</title>
+
+ <para>
+ <command>REPACK</command> reclaims storage occupied by dead
+ tuples. Unlike <command>VACUUM</command>, it does so by rewriting the
+ entire contents of the table specified
+ by <replaceable class=3D"parameter">table_name</replaceable> into a new=
disk
+ file with no extra space (except for the space guaranteed by
+ the <literal>fillfactor</literal> storage parameter), allowing unused s=
pace
+ to be returned to the operating system.
+ </para>
+
+ <para>
+ Without
+ a <replaceable class=3D"parameter">table_name</replaceable>, <command>R=
EPACK</command>
+ processes every table and materialized view in the current database that
+ the current user has the <literal>MAINTAIN</literal> privilege on. This
+ form of <command>REPACK</command> cannot be executed inside a transacti=
on
+ block.
+ </para>
+
+ <para>
+ If a <literal>USING INDEX</literal> clause is specified, the rows are
+ physically reordered based on information from an index. Please see the
+ notes on clustering below.
+ </para>
+
+ <para>
+ When a table is being repacked, an <literal>ACCESS EXCLUSIVE</literal> =
lock
+ is acquired on it. This prevents any other database operations (both re=
ads
+ and writes) from operating on the table until the <command>REPACK</comm=
and>
+ is finished.
+ </para>
+
+ <refsect2 id=3D"sql-repack-notes-on-clustering" xreflabel=3D"Notes on Cl=
ustering">
+ <title>Notes on Clustering</title>
+
+ <para>
+ If the <literal>USING INDEX</literal> clause is specified, the rows in
+ the table are physically reordered following an index: if an index name
+ is specified in the command, then that index is used; if no index name
+ is specified, then the index that has been configured as the index to
+ cluster on. If no index has been configured in this way, an error is
+ thrown. The index given in the <literal>USING INDEX</literal> clause
+ is configured as the index to cluster on, as well as an index given
+ to the <command>CLUSTER</command> command. An index can be set
+ manually using <command>ALTER TABLE ... CLUSTER ON</command>, and reset
+ with <command>ALTER TABLE ... SET WITHOUT CLUSTER</command>.
+ </para>
+
+ <para>
+ If no table name is specified in <command>REPACK USING INDEX</command>,
+ all tables which have a clustering index defined and which the calling
+ user has privileges for are processed.
+ </para>
+
+ <para>
+ Clustering is a one-time operation: when the table is
+ subsequently updated, the changes are not clustered. That is, no atte=
mpt
+ is made to store new or updated rows according to their index order. =
(If
+ one wishes, one can periodically recluster by issuing the command agai=
n.
+ Also, setting the table's <literal>fillfactor</literal> storage parame=
ter
+ to less than 100% can aid in preserving cluster ordering during update=
s,
+ since updated rows are kept on the same page if enough space is availa=
ble
+ there.)
+ </para>
+
+ <para>
+ In cases where you are accessing single rows randomly within a table, =
the
+ actual order of the data in the table is unimportant. However, if you =
tend
+ to access some data more than others, and there is an index that groups
+ them together, you will benefit from using clustering. If
+ you are requesting a range of indexed values from a table, or a single
+ indexed value that has multiple rows that match,
+ <command>REPACK</command> will help because once the index identifies =
the
+ table page for the first row that matches, all other rows that match a=
re
+ probably already on the same table page, and so you save disk accesses=
and
+ speed up the query.
+ </para>
+
+ <para>
+ <command>REPACK</command> can re-sort the table using either an index =
scan
+ on the specified index (if the index is a b-tree), or a sequential scan
+ followed by sorting. It will attempt to choose the method that will be
+ faster, based on planner cost parameters and available statistical
+ information.
+ </para>
+
+ <para>
+ Because the planner records statistics about the ordering of tables, i=
t is
+ advisable to
+ run <link linkend=3D"sql-analyze"><command>ANALYZE</command></link> on=
the
+ newly repacked table. Otherwise, the planner might make poor choices =
of
+ query plans.
+ </para>
+ </refsect2>
+
+ <refsect2 id=3D"sql-repack-notes-on-resources" xreflabel=3D"Notes on Res=
ources">
+ <title>Notes on Resources</title>
+
+ <para>
+ When an index scan or a sequential scan without sort is used, a tempor=
ary
+ copy of the table is created that contains the table data in the index
+ order. Temporary copies of each index on the table are created as wel=
l.
+ Therefore, you need free space on disk at least equal to the sum of the
+ table size and the index sizes.
+ </para>
+
+ <para>
+ When a sequential scan and sort is used, a temporary sort file is also
+ created, so that the peak temporary space requirement is as much as do=
uble
+ the table size, plus the index sizes. This method is often faster than
+ the index scan method, but if the disk space requirement is intolerabl=
e,
+ you can disable this choice by temporarily setting
+ <xref linkend=3D"guc-enable-sort"/> to <literal>off</literal>.
+ </para>
+
+ <para>
+ It is advisable to set <xref linkend=3D"guc-maintenance-work-mem"/> to=
a
+ reasonably large value (but not more than the amount of RAM you can
+ dedicate to the <command>REPACK</command> operation) before repacking.
+ </para>
+ </refsect2>
+
+ </refsect1>
+
+ <refsect1>
+ <title>Parameters</title>
+
+ <variablelist>
+ <varlistentry>
+ <term><replaceable class=3D"parameter">table_name</replaceable></term>
+ <listitem>
+ <para>
+ The name (possibly schema-qualified) of a table.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class=3D"parameter">column_name</replaceable></term>
+ <listitem>
+ <para>
+ The name of a specific column to analyze. Defaults to all columns.
+ If a column list is specific, <literal>ANALYZE</literal> must also
+ be specified.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class=3D"parameter">index_name</replaceable></term>
+ <listitem>
+ <para>
+ The name of an index.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>VERBOSE</literal></term>
+ <listitem>
+ <para>
+ Prints a progress report as each table is repacked
+ at <literal>INFO</literal> level.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>ANALYZE</literal></term>
+ <term><literal>ANALYSE</literal></term>
+ <listitem>
+ <para>
+ Applies <xref linkend=3D"sql-analyze"/> on the table after repacking=
. This is
+ currently only supported when a single (non-partitioned) table is sp=
ecified.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class=3D"parameter">boolean</replaceable></term>
+ <listitem>
+ <para>
+ Specifies whether the selected option should be turned on or off.
+ You can write <literal>TRUE</literal>, <literal>ON</literal>, or
+ <literal>1</literal> to enable the option, and <literal>FALSE</liter=
al>,
+ <literal>OFF</literal>, or <literal>0</literal> to disable it. The
+ <replaceable class=3D"parameter">boolean</replaceable> value can also
+ be omitted, in which case <literal>TRUE</literal> is assumed.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </refsect1>
+
+ <refsect1>
+ <title>Notes</title>
+
+ <para>
+ To repack a table, one must have the <literal>MAINTAIN</literal> privi=
lege
+ on the table.
+ </para>
+
+ <para>
+ While <command>REPACK</command> is running, the <xref
+ linkend=3D"guc-search-path"/> is temporarily changed to <literal>pg_ca=
talog,
+ pg_temp</literal>.
+ </para>
+
+ <para>
+ Each backend running <command>REPACK</command> will report its progress
+ in the <structname>pg_stat_progress_repack</structname> view. See
+ <xref linkend=3D"repack-progress-reporting"/> for details.
+ </para>
+
+ <para>
+ Repacking a partitioned table repacks each of its partitions. If an in=
dex
+ is specified, each partition is repacked using the partition of that
+ index. <command>REPACK</command> on a partitioned table cannot be exec=
uted
+ inside a transaction block.
+ </para>
+
+ </refsect1>
+
+ <refsect1>
+ <title>Examples</title>
+
+ <para>
+ Repack the table <literal>employees</literal>:
+<programlisting>
+REPACK employees;
+</programlisting>
+ </para>
+
+ <para>
+ Repack the table <literal>employees</literal> on the basis of its
+ index <literal>employees_ind</literal> (Since index is used here, this =
is
+ effectively clustering):
+<programlisting>
+REPACK employees USING INDEX employees_ind;
+</programlisting>
+ </para>
+
+ <para>
+ Repack the table <literal>cases</literal> on physical ordering,
+ running an <command>ANALYZE</command> on the given columns once
+ repacking is done, showing informational messages:
+<programlisting>
+REPACK (ANALYZE, VERBOSE) cases (district, case_nr);
+</programlisting>
+ </para>
+
+ <para>
+ Repack all tables in the database on which you have
+ the <literal>MAINTAIN</literal> privilege:
+<programlisting>
+REPACK;
+</programlisting>
+ </para>
+
+ <para>
+ Repack all tables for which a clustering index has previously been
+ configured on which you have the <literal>MAINTAIN</literal> privilege,
+ showing informational messages:
+<programlisting>
+REPACK (VERBOSE) USING INDEX;
+</programlisting>
+ </para>
+
+ </refsect1>
+
+ <refsect1>
+ <title>Compatibility</title>
+
+ <para>
+ There is no <command>REPACK</command> statement in the SQL standard.
+ </para>
+ </refsect1>
+
+ <refsect1>
+ <title>See Also</title>
+
+ <simplelist type=3D"inline">
+ <member><xref linkend=3D"app-pgrepackdb"/></member>
+ <member><xref linkend=3D"repack-progress-reporting"/></member>
+ </simplelist>
+ </refsect1>
+
+</refentry>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 6d0fdd43cfb..ac5d083d468 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -25,7 +25,6 @@ VACUUM [ ( <replaceable class=3D"parameter">option</repla=
ceable> [, ...] ) ] [ <re
=20
<phrase>where <replaceable class=3D"parameter">option</replaceable> can be=
one of:</phrase>
=20
- FULL [ <replaceable class=3D"parameter">boolean</replaceable> ]
FREEZE [ <replaceable class=3D"parameter">boolean</replaceable> ]
VERBOSE [ <replaceable class=3D"parameter">boolean</replaceable> ]
ANALYZE [ <replaceable class=3D"parameter">boolean</replaceable> ]
@@ -39,6 +38,7 @@ VACUUM [ ( <replaceable class=3D"parameter">option</repla=
ceable> [, ...] ) ] [ <re
SKIP_DATABASE_STATS [ <replaceable class=3D"parameter">boolean</replac=
eable> ]
ONLY_DATABASE_STATS [ <replaceable class=3D"parameter">boolean</replac=
eable> ]
BUFFER_USAGE_LIMIT <replaceable class=3D"parameter">size</replaceable>
+ FULL [ <replaceable class=3D"parameter">boolean</replaceable> ]
=20
<phrase>and <replaceable class=3D"parameter">table_and_columns</replaceabl=
e> is:</phrase>
=20
@@ -95,20 +95,6 @@ VACUUM [ ( <replaceable class=3D"parameter">option</repl=
aceable> [, ...] ) ] [ <re
<title>Parameters</title>
=20
<variablelist>
- <varlistentry>
- <term><literal>FULL</literal></term>
- <listitem>
- <para>
- Selects <quote>full</quote> vacuum, which can reclaim more
- space, but takes much longer and exclusively locks the table.
- This method also requires extra disk space, since it writes a
- new copy of the table and doesn't release the old copy until
- the operation is complete. Usually this should only be used when a
- significant amount of space needs to be reclaimed from within the ta=
ble.
- </para>
- </listitem>
- </varlistentry>
-
<varlistentry>
<term><literal>FREEZE</literal></term>
<listitem>
@@ -362,6 +348,23 @@ VACUUM [ ( <replaceable class=3D"parameter">option</re=
placeable> [, ...] ) ] [ <re
</listitem>
</varlistentry>
=20
+ <varlistentry>
+ <term><literal>FULL</literal></term>
+ <listitem>
+ <para>
+ This option, which is deprecated, makes <command>VACUUM</command>
+ behave like <command>REPACK</command> without a
+ <literal>USING INDEX</literal> clause.
+ This method of compacting the table takes much longer than
+ <command>VACUUM</command> and exclusively locks the table.
+ This method also requires extra disk space, since it writes a
+ new copy of the table and doesn't release the old copy until
+ the operation is complete. Usually this should only be used when a
+ significant amount of space needs to be reclaimed from within the ta=
ble.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class=3D"parameter">boolean</replaceable></term>
<listitem>
diff --git a/doc/src/sgml/reference.sgml b/doc/src/sgml/reference.sgml
index 2cf02c37b17..d9fdbb5d254 100644
--- a/doc/src/sgml/reference.sgml
+++ b/doc/src/sgml/reference.sgml
@@ -195,6 +195,7 @@
&refreshMaterializedView;
&reindex;
&releaseSavepoint;
+ &repack;
&reset;
&revoke;
&rollback;
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/=
heap/heapam_handler.c
index cbef73e5d4b..7d4b48e5a97 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -741,13 +741,13 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Re=
lation NewHeap,
if (OldIndex !=3D NULL && !use_sort)
{
const int ci_index[] =3D {
- PROGRESS_CLUSTER_PHASE,
- PROGRESS_CLUSTER_INDEX_RELID
+ PROGRESS_REPACK_PHASE,
+ PROGRESS_REPACK_INDEX_RELID
};
int64 ci_val[2];
=20
/* Set phase and OIDOldIndex to columns */
- ci_val[0] =3D PROGRESS_CLUSTER_PHASE_INDEX_SCAN_HEAP;
+ ci_val[0] =3D PROGRESS_REPACK_PHASE_INDEX_SCAN_HEAP;
ci_val[1] =3D RelationGetRelid(OldIndex);
pgstat_progress_update_multi_param(2, ci_index, ci_val);
=20
@@ -759,15 +759,15 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Re=
lation NewHeap,
else
{
/* In scan-and-sort mode and also VACUUM FULL, set phase */
- pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
- PROGRESS_CLUSTER_PHASE_SEQ_SCAN_HEAP);
+ pgstat_progress_update_param(PROGRESS_REPACK_PHASE,
+ PROGRESS_REPACK_PHASE_SEQ_SCAN_HEAP);
=20
tableScan =3D table_beginscan(OldHeap, SnapshotAny, 0, (ScanKey) NULL);
heapScan =3D (HeapScanDesc) tableScan;
indexScan =3D NULL;
=20
/* Set total heap blocks */
- pgstat_progress_update_param(PROGRESS_CLUSTER_TOTAL_HEAP_BLKS,
+ pgstat_progress_update_param(PROGRESS_REPACK_TOTAL_HEAP_BLKS,
heapScan->rs_nblocks);
}
=20
@@ -809,7 +809,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Rela=
tion NewHeap,
* is manually updated to the correct value when the table
* scan finishes.
*/
- pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_BLKS_SCANNED,
+ pgstat_progress_update_param(PROGRESS_REPACK_HEAP_BLKS_SCANNED,
heapScan->rs_nblocks);
break;
}
@@ -825,7 +825,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Rela=
tion NewHeap,
*/
if (prev_cblock !=3D heapScan->rs_cblock)
{
- pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_BLKS_SCANNED,
+ pgstat_progress_update_param(PROGRESS_REPACK_HEAP_BLKS_SCANNED,
(heapScan->rs_cblock +
heapScan->rs_nblocks -
heapScan->rs_startblock
@@ -926,14 +926,14 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Re=
lation NewHeap,
* In scan-and-sort mode, report increase in number of tuples
* scanned
*/
- pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_TUPLES_SCANNED,
+ pgstat_progress_update_param(PROGRESS_REPACK_HEAP_TUPLES_SCANNED,
*num_tuples);
}
else
{
const int ct_index[] =3D {
- PROGRESS_CLUSTER_HEAP_TUPLES_SCANNED,
- PROGRESS_CLUSTER_HEAP_TUPLES_WRITTEN
+ PROGRESS_REPACK_HEAP_TUPLES_SCANNED,
+ PROGRESS_REPACK_HEAP_TUPLES_WRITTEN
};
int64 ct_val[2];
=20
@@ -966,14 +966,14 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Re=
lation NewHeap,
double n_tuples =3D 0;
=20
/* Report that we are now sorting tuples */
- pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
- PROGRESS_CLUSTER_PHASE_SORT_TUPLES);
+ pgstat_progress_update_param(PROGRESS_REPACK_PHASE,
+ PROGRESS_REPACK_PHASE_SORT_TUPLES);
=20
tuplesort_performsort(tuplesort);
=20
/* Report that we are now writing new heap */
- pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
- PROGRESS_CLUSTER_PHASE_WRITE_NEW_HEAP);
+ pgstat_progress_update_param(PROGRESS_REPACK_PHASE,
+ PROGRESS_REPACK_PHASE_WRITE_NEW_HEAP);
=20
for (;;)
{
@@ -991,7 +991,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Rela=
tion NewHeap,
values, isnull,
rwstate);
/* Report n_tuples */
- pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_TUPLES_WRITTEN,
+ pgstat_progress_update_param(PROGRESS_REPACK_HEAP_TUPLES_WRITTEN,
n_tuples);
}
=20
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 43de42ce39e..5ee6389d39c 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -4077,7 +4077,7 @@ reindex_relation(const ReindexStmt *stmt, Oid relid, =
int flags,
Assert(!ReindexIsProcessingIndex(indexOid));
=20
/* Set index rebuild count */
- pgstat_progress_update_param(PROGRESS_CLUSTER_INDEX_REBUILD_COUNT,
+ pgstat_progress_update_param(PROGRESS_REPACK_INDEX_REBUILD_COUNT,
i);
i++;
}
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/sys=
tem_views.sql
index 1ea8f1faa9e..201c503548e 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1304,14 +1304,15 @@ CREATE VIEW pg_stat_progress_vacuum AS
FROM pg_stat_get_progress_info('VACUUM') AS S
LEFT JOIN pg_database D ON S.datid =3D D.oid;
=20
-CREATE VIEW pg_stat_progress_cluster AS
+CREATE VIEW pg_stat_progress_repack AS
SELECT
S.pid AS pid,
S.datid AS datid,
D.datname AS datname,
S.relid AS relid,
CASE S.param1 WHEN 1 THEN 'CLUSTER'
- WHEN 2 THEN 'VACUUM FULL'
+ WHEN 2 THEN 'REPACK'
+ WHEN 3 THEN 'VACUUM FULL'
END AS command,
CASE S.param2 WHEN 0 THEN 'initializing'
WHEN 1 THEN 'seq scanning heap'
@@ -1322,15 +1323,35 @@ CREATE VIEW pg_stat_progress_cluster AS
WHEN 6 THEN 'rebuilding index'
WHEN 7 THEN 'performing final cleanup'
END AS phase,
- CAST(S.param3 AS oid) AS cluster_index_relid,
+ CAST(S.param3 AS oid) AS repack_index_relid,
S.param4 AS heap_tuples_scanned,
S.param5 AS heap_tuples_written,
S.param6 AS heap_blks_total,
S.param7 AS heap_blks_scanned,
S.param8 AS index_rebuild_count
- FROM pg_stat_get_progress_info('CLUSTER') AS S
+ FROM pg_stat_get_progress_info('REPACK') AS S
LEFT JOIN pg_database D ON S.datid =3D D.oid;
=20
+-- This view is as the one above, except for renaming a column and avoiding
+-- 'REPACK' as a command name to report.
+CREATE VIEW pg_stat_progress_cluster AS
+ SELECT
+ pid,
+ datid,
+ datname,
+ relid,
+ CASE WHEN command IN ('CLUSTER', 'VACUUM FULL') THEN command
+ WHEN repack_index_relid =3D 0 THEN 'VACUUM FULL'
+ ELSE 'CLUSTER' END AS command,
+ phase,
+ repack_index_relid AS cluster_index_relid,
+ heap_tuples_scanned,
+ heap_tuples_written,
+ heap_blks_total,
+ heap_blks_scanned,
+ index_rebuild_count
+ FROM pg_stat_progress_repack;
+
CREATE VIEW pg_stat_progress_create_index AS
SELECT
S.pid AS pid, S.datid AS datid, D.datname AS datname,
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 60a4617a585..d70636a6a63 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* cluster.c
- * CLUSTER a table on an index. This is now also used for VACUUM FULL.
+ * CLUSTER a table on an index. This is now also used for VACUUM FULL a=
nd
+ * REPACK.
*
* There is hardly anything left of Paul Brown's original implementation...
*
@@ -67,27 +68,35 @@ typedef struct
Oid indexOid;
} RelToCluster;
=20
-
-static void cluster_multiple_rels(List *rtcs, ClusterParams *params);
+static bool cluster_rel_recheck(RepackCommand cmd, Relation OldHeap,
+ Oid indexOid, Oid userid, int options);
static void rebuild_relation(Relation OldHeap, Relation index, bool verbos=
e);
static void copy_table_data(Relation NewHeap, Relation OldHeap, Relation O=
ldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
-static List *get_tables_to_cluster(MemoryContext cluster_context);
-static List *get_tables_to_cluster_partitioned(MemoryContext cluster_conte=
xt,
- Oid indexOid);
-static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
+static List *get_tables_to_repack(RepackCommand cmd, bool usingindex,
+ MemoryContext permcxt);
+static List *get_tables_to_repack_partitioned(RepackCommand cmd,
+ Oid relid, bool rel_is_index,
+ MemoryContext permcxt);
+static bool cluster_is_permitted_for_relation(RepackCommand cmd,
+ Oid relid, Oid userid);
+static Relation process_single_relation(RepackStmt *stmt,
+ ClusterParams *params);
+static Oid determine_clustered_index(Relation rel, bool usingindex,
+ const char *indexname);
+static const char *RepackCommandAsString(RepackCommand cmd);
=20
=20
-/*------------------------------------------------------------------------=
---
- * This cluster code allows for clustering multiple tables at once. Because
+/*
+ * The repack code allows for processing multiple tables at once. Because
* of this, we cannot just run everything on a single transaction, or we
* would be forced to acquire exclusive locks on all the tables being
* clustered, simultaneously --- very likely leading to deadlock.
*
- * To solve this we follow a similar strategy to VACUUM code,
- * clustering each relation in a separate transaction. For this to work,
- * we need to:
+ * To solve this we follow a similar strategy to VACUUM code, processing e=
ach
+ * relation in a separate transaction. For this to work, we need to:
+ *
* - provide a separate memory context so that we can pass information in
* a way that survives across transactions
* - start a new transaction every time a new relation is clustered
@@ -98,197 +107,166 @@ static bool cluster_is_permitted_for_relation(Oid re=
lid, Oid userid);
*
* The single-relation case does not have any such overhead.
*
- * We also allow a relation to be specified without index. In that case,
- * the indisclustered bit will be looked up, and an ERROR will be thrown
- * if there is no index with the bit set.
- *------------------------------------------------------------------------=
---
+ * We also allow a relation to be repacked following an index, but without
+ * naming a specific one. In that case, the indisclustered bit will be
+ * looked up, and an ERROR will be thrown if no so-marked index is found.
*/
void
-cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
+ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel)
{
- ListCell *lc;
ClusterParams params =3D {0};
- bool verbose =3D false;
Relation rel =3D NULL;
- Oid indexOid =3D InvalidOid;
- MemoryContext cluster_context;
+ MemoryContext repack_context;
List *rtcs;
=20
/* Parse option list */
- foreach(lc, stmt->params)
+ foreach_node(DefElem, opt, stmt->params)
{
- DefElem *opt =3D (DefElem *) lfirst(lc);
-
if (strcmp(opt->defname, "verbose") =3D=3D 0)
- verbose =3D defGetBoolean(opt);
+ params.options |=3D defGetBoolean(opt) ? CLUOPT_VERBOSE : 0;
+ else if (strcmp(opt->defname, "analyze") =3D=3D 0 ||
+ strcmp(opt->defname, "analyse") =3D=3D 0)
+ params.options |=3D defGetBoolean(opt) ? CLUOPT_ANALYZE : 0;
else
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("unrecognized %s option \"%s\"",
- "CLUSTER", opt->defname),
- parser_errposition(pstate, opt->location)));
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("unrecognized %s option \"%s\"",
+ RepackCommandAsString(stmt->command),
+ opt->defname),
+ parser_errposition(pstate, opt->location));
}
=20
- params.options =3D (verbose ? CLUOPT_VERBOSE : 0);
-
+ /*
+ * If a single relation is specified, process it and we're done ... unless
+ * the relation is a partitioned table, in which case we fall through.
+ */
if (stmt->relation !=3D NULL)
{
- /* This is the single-relation case. */
- Oid tableOid;
-
- /*
- * Find, lock, and check permissions on the table. We obtain
- * AccessExclusiveLock right away to avoid lock-upgrade hazard in the
- * single-transaction case.
- */
- tableOid =3D RangeVarGetRelidExtended(stmt->relation,
- AccessExclusiveLock,
- 0,
- RangeVarCallbackMaintainsTable,
- NULL);
- rel =3D table_open(tableOid, NoLock);
-
- /*
- * Reject clustering a remote temp table ... their local buffer
- * manager is not going to cope.
- */
- if (RELATION_IS_OTHER_TEMP(rel))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot cluster temporary tables of other sessions")));
-
- if (stmt->indexname =3D=3D NULL)
- {
- ListCell *index;
-
- /* We need to find the index that has indisclustered set. */
- foreach(index, RelationGetIndexList(rel))
- {
- indexOid =3D lfirst_oid(index);
- if (get_index_isclustered(indexOid))
- break;
- indexOid =3D InvalidOid;
- }
-
- if (!OidIsValid(indexOid))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("there is no previously clustered index for table \"%s\"",
- stmt->relation->relname)));
- }
- else
- {
- /*
- * The index is expected to be in the same namespace as the
- * relation.
- */
- indexOid =3D get_relname_relid(stmt->indexname,
- rel->rd_rel->relnamespace);
- if (!OidIsValid(indexOid))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("index \"%s\" for table \"%s\" does not exist",
- stmt->indexname, stmt->relation->relname)));
- }
-
- /* For non-partitioned tables, do what we came here to do. */
- if (rel->rd_rel->relkind !=3D RELKIND_PARTITIONED_TABLE)
- {
- cluster_rel(rel, indexOid, ¶ms);
- /* cluster_rel closes the relation, but keeps lock */
-
- return;
- }
+ rel =3D process_single_relation(stmt, ¶ms);
+ if (rel =3D=3D NULL)
+ return; /* all done */
}
=20
+ /*
+ * Don't allow ANALYZE in the multiple-relation case for now. Maybe we
+ * can add support for this later.
+ */
+ if (params.options & CLUOPT_ANALYZE)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot %s multiple tables", "REPACK (ANALYZE)"));
+
/*
* By here, we know we are in a multi-table situation. In order to avoid
* holding locks for too long, we want to process each table in its own
* transaction. This forces us to disallow running inside a user
* transaction block.
*/
- PreventInTransactionBlock(isTopLevel, "CLUSTER");
+ PreventInTransactionBlock(isTopLevel, RepackCommandAsString(stmt->command=
));
=20
/* Also, we need a memory context to hold our list of relations */
- cluster_context =3D AllocSetContextCreate(PortalContext,
- "Cluster",
- ALLOCSET_DEFAULT_SIZES);
+ repack_context =3D AllocSetContextCreate(PortalContext,
+ "Repack",
+ ALLOCSET_DEFAULT_SIZES);
+
+ params.options |=3D CLUOPT_RECHECK;
=20
/*
- * Either we're processing a partitioned table, or we were not given any
- * table name at all. In either case, obtain a list of relations to
- * process.
- *
- * In the former case, an index name must have been given, so we don't
- * need to recheck its "indisclustered" bit, but we have to check that it
- * is an index that we can cluster on. In the latter case, we set the
- * option bit to have indisclustered verified.
- *
- * Rechecking the relation itself is necessary here in all cases.
+ * If we don't have a relation yet, determine a relation list. If we do,
+ * then it must be a partitioned table, and we want to process its
+ * partitions.
*/
- params.options |=3D CLUOPT_RECHECK;
- if (rel !=3D NULL)
+ if (rel =3D=3D NULL)
{
- Assert(rel->rd_rel->relkind =3D=3D RELKIND_PARTITIONED_TABLE);
- check_index_is_clusterable(rel, indexOid, AccessShareLock);
- rtcs =3D get_tables_to_cluster_partitioned(cluster_context, indexOid);
-
- /* close relation, releasing lock on parent table */
- table_close(rel, AccessExclusiveLock);
+ Assert(stmt->indexname =3D=3D NULL);
+ rtcs =3D get_tables_to_repack(stmt->command, stmt->usingindex,
+ repack_context);
+ params.options |=3D CLUOPT_RECHECK_ISCLUSTERED;
}
else
{
- rtcs =3D get_tables_to_cluster(cluster_context);
- params.options |=3D CLUOPT_RECHECK_ISCLUSTERED;
- }
+ Oid relid;
+ bool rel_is_index;
=20
- /* Do the job. */
- cluster_multiple_rels(rtcs, ¶ms);
+ Assert(rel->rd_rel->relkind =3D=3D RELKIND_PARTITIONED_TABLE);
=20
- /* Start a new transaction for the cleanup work. */
- StartTransactionCommand();
+ /*
+ * If USING INDEX was specified, resolve the index name now and pass
+ * it down.
+ */
+ if (stmt->usingindex)
+ {
+ /*
+ * If no index name was specified when repacking a partitioned
+ * table, punt for now. Maybe we can improve this later.
+ */
+ if (!stmt->indexname)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("there is no previously clustered index for table \"%s\"",
+ RelationGetRelationName(rel)));
+
+ relid =3D determine_clustered_index(rel, stmt->usingindex,
+ stmt->indexname);
+ if (!OidIsValid(relid))
+ elog(ERROR, "unable to determine index to cluster on");
+ /* XXX is this the right place for this check? */
+ check_index_is_clusterable(rel, relid, AccessExclusiveLock);
+ rel_is_index =3D true;
+ }
+ else
+ {
+ relid =3D RelationGetRelid(rel);
+ rel_is_index =3D false;
+ }
=20
- /* Clean up working storage */
- MemoryContextDelete(cluster_context);
-}
+ rtcs =3D get_tables_to_repack_partitioned(stmt->command,
+ relid, rel_is_index,
+ repack_context);
=20
-/*
- * Given a list of relations to cluster, process each of them in a separate
- * transaction.
- *
- * We expect to be in a transaction at start, but there isn't one when we
- * return.
- */
-static void
-cluster_multiple_rels(List *rtcs, ClusterParams *params)
-{
- ListCell *lc;
+ /* close parent relation, releasing lock on it */
+ table_close(rel, AccessExclusiveLock);
+ rel =3D NULL;
+ }
=20
/* Commit to get out of starting transaction */
PopActiveSnapshot();
CommitTransactionCommand();
=20
/* Cluster the tables, each in a separate transaction */
- foreach(lc, rtcs)
+ Assert(rel =3D=3D NULL);
+ foreach_ptr(RelToCluster, rtc, rtcs)
{
- RelToCluster *rtc =3D (RelToCluster *) lfirst(lc);
- Relation rel;
-
/* Start a new transaction for each relation. */
StartTransactionCommand();
=20
+ /*
+ * Open the target table, coping with the case where it has been
+ * dropped.
+ */
+ rel =3D try_table_open(rtc->tableOid, AccessExclusiveLock);
+ if (rel =3D=3D NULL)
+ {
+ CommitTransactionCommand();
+ continue;
+ }
+
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
=20
- rel =3D table_open(rtc->tableOid, AccessExclusiveLock);
-
/* Process this table */
- cluster_rel(rel, rtc->indexOid, params);
+ cluster_rel(stmt->command, rel, rtc->indexOid, ¶ms);
/* cluster_rel closes the relation, but keeps lock */
=20
PopActiveSnapshot();
CommitTransactionCommand();
}
+
+ /* Start a new transaction for the cleanup work. */
+ StartTransactionCommand();
+
+ /* Clean up working storage */
+ MemoryContextDelete(repack_context);
}
=20
/*
@@ -304,11 +282,14 @@ cluster_multiple_rels(List *rtcs, ClusterParams *para=
ms)
* them incrementally while we load the table.
*
* If indexOid is InvalidOid, the table will be rewritten in physical order
- * instead of index order. This is the new implementation of VACUUM FULL,
- * and error messages should refer to the operation as VACUUM not CLUSTER.
+ * instead of index order.
+ *
+ * 'cmd' indicates which command is being executed, to be used for error
+ * messages.
*/
void
-cluster_rel(Relation OldHeap, Oid indexOid, ClusterParams *params)
+cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid,
+ ClusterParams *params)
{
Oid tableOid =3D RelationGetRelid(OldHeap);
Oid save_userid;
@@ -323,13 +304,8 @@ cluster_rel(Relation OldHeap, Oid indexOid, ClusterPar=
ams *params)
/* Check for user-requested abort. */
CHECK_FOR_INTERRUPTS();
=20
- pgstat_progress_start_command(PROGRESS_COMMAND_CLUSTER, tableOid);
- if (OidIsValid(indexOid))
- pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
- PROGRESS_CLUSTER_COMMAND_CLUSTER);
- else
- pgstat_progress_update_param(PROGRESS_CLUSTER_COMMAND,
- PROGRESS_CLUSTER_COMMAND_VACUUM_FULL);
+ pgstat_progress_start_command(PROGRESS_COMMAND_REPACK, tableOid);
+ pgstat_progress_update_param(PROGRESS_REPACK_COMMAND, cmd);
=20
/*
* Switch to the table owner's userid, so that any index functions are run
@@ -350,86 +326,38 @@ cluster_rel(Relation OldHeap, Oid indexOid, ClusterPa=
rams *params)
* *must* skip the one on indisclustered since it would reject an attempt
* to cluster a not-previously-clustered index.
*/
- if (recheck)
- {
- /* Check that the user still has privileges for the relation */
- if (!cluster_is_permitted_for_relation(tableOid, save_userid))
- {
- relation_close(OldHeap, AccessExclusiveLock);
- goto out;
- }
-
- /*
- * Silently skip a temp table for a remote session. Only doing this
- * check in the "recheck" case is appropriate (which currently means
- * somebody is executing a database-wide CLUSTER or on a partitioned
- * table), because there is another check in cluster() which will stop
- * any attempt to cluster remote temp tables by name. There is
- * another check in cluster_rel which is redundant, but we leave it
- * for extra safety.
- */
- if (RELATION_IS_OTHER_TEMP(OldHeap))
- {
- relation_close(OldHeap, AccessExclusiveLock);
- goto out;
- }
-
- if (OidIsValid(indexOid))
- {
- /*
- * Check that the index still exists
- */
- if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(indexOid)))
- {
- relation_close(OldHeap, AccessExclusiveLock);
- goto out;
- }
-
- /*
- * Check that the index is still the one with indisclustered set,
- * if needed.
- */
- if ((params->options & CLUOPT_RECHECK_ISCLUSTERED) !=3D 0 &&
- !get_index_isclustered(indexOid))
- {
- relation_close(OldHeap, AccessExclusiveLock);
- goto out;
- }
- }
- }
+ if (recheck &&
+ !cluster_rel_recheck(cmd, OldHeap, indexOid, save_userid,
+ params->options))
+ goto out;
=20
/*
- * We allow VACUUM FULL, but not CLUSTER, on shared catalogs. CLUSTER
- * would work in most respects, but the index would only get marked as
- * indisclustered in the current database, leading to unexpected behavior
- * if CLUSTER were later invoked in another database.
+ * We allow repacking shared catalogs only when not using an index. It
+ * would work to use an index in most respects, but the index would only
+ * get marked as indisclustered in the current database, leading to
+ * unexpected behavior if CLUSTER were later invoked in another database.
*/
if (OidIsValid(indexOid) && OldHeap->rd_rel->relisshared)
ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot cluster a shared catalog")));
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot run %s on a shared catalog",
+ RepackCommandAsString(cmd)));
=20
/*
* Don't process temp tables of other backends ... their local buffer
* manager is not going to cope.
*/
if (RELATION_IS_OTHER_TEMP(OldHeap))
- {
- if (OidIsValid(indexOid))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot cluster temporary tables of other sessions")));
- else
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot vacuum temporary tables of other sessions")));
- }
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot run %s on temporary tables of other sessions",
+ RepackCommandAsString(cmd)));
=20
/*
* Also check for active uses of the relation in the current transaction,
* including open scans and pending AFTER trigger events.
*/
- CheckTableNotInUse(OldHeap, OidIsValid(indexOid) ? "CLUSTER" : "VACUUM");
+ CheckTableNotInUse(OldHeap, RepackCommandAsString(cmd));
=20
/* Check heap and index are valid to cluster on */
if (OidIsValid(indexOid))
@@ -442,6 +370,24 @@ cluster_rel(Relation OldHeap, Oid indexOid, ClusterPar=
ams *params)
else
index =3D NULL;
=20
+ /*
+ * When allow_system_table_mods is turned off, we disallow repacking a
+ * catalog on a particular index unless that's already the clustered index
+ * for that catalog.
+ *
+ * XXX We don't check for this in CLUSTER, because it's historically been
+ * allowed.
+ */
+ if (cmd !=3D REPACK_COMMAND_CLUSTER &&
+ !allowSystemTableMods && OidIsValid(indexOid) &&
+ IsCatalogRelation(OldHeap) && !index->rd_index->indisclustered)
+ ereport(ERROR,
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a system catalog",
+ RelationGetRelationName(OldHeap)),
+ errdetail("System catalogs can only be clustered by the index they're =
already clustered on, if any, unless \"%s\" is enabled.",
+ "allow_system_table_mods"));
+
/*
* Quietly ignore the request if this is a materialized view which has not
* been populated from its query. No harm is done because there is no data
@@ -482,6 +428,63 @@ out:
pgstat_progress_end_command();
}
=20
+/*
+ * Check if the table (and its index) still meets the requirements of
+ * cluster_rel().
+ */
+static bool
+cluster_rel_recheck(RepackCommand cmd, Relation OldHeap, Oid indexOid,
+ Oid userid, int options)
+{
+ Oid tableOid =3D RelationGetRelid(OldHeap);
+
+ /* Check that the user still has privileges for the relation */
+ if (!cluster_is_permitted_for_relation(cmd, tableOid, userid))
+ {
+ relation_close(OldHeap, AccessExclusiveLock);
+ return false;
+ }
+
+ /*
+ * Silently skip a temp table for a remote session. Only doing this check
+ * in the "recheck" case is appropriate (which currently means somebody is
+ * executing a database-wide CLUSTER or on a partitioned table), because
+ * there is another check in cluster() which will stop any attempt to
+ * cluster remote temp tables by name. There is another check in
+ * cluster_rel which is redundant, but we leave it for extra safety.
+ */
+ if (RELATION_IS_OTHER_TEMP(OldHeap))
+ {
+ relation_close(OldHeap, AccessExclusiveLock);
+ return false;
+ }
+
+ if (OidIsValid(indexOid))
+ {
+ /*
+ * Check that the index still exists
+ */
+ if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(indexOid)))
+ {
+ relation_close(OldHeap, AccessExclusiveLock);
+ return false;
+ }
+
+ /*
+ * Check that the index is still the one with indisclustered set, if
+ * needed.
+ */
+ if ((options & CLUOPT_RECHECK_ISCLUSTERED) !=3D 0 &&
+ !get_index_isclustered(indexOid))
+ {
+ relation_close(OldHeap, AccessExclusiveLock);
+ return false;
+ }
+ }
+
+ return true;
+}
+
/*
* Verify that the specified heap and index are valid to cluster on
*
@@ -642,8 +645,8 @@ rebuild_relation(Relation OldHeap, Relation index, bool=
verbose)
Assert(CheckRelationLockedByMe(OldHeap, AccessExclusiveLock, false) &&
(index =3D=3D NULL || CheckRelationLockedByMe(index, AccessExclusiveL=
ock, false)));
=20
- if (index)
- /* Mark the correct index as clustered */
+ /* for CLUSTER or REPACK USING INDEX, mark the index as the one to use */
+ if (index !=3D NULL)
mark_index_clustered(OldHeap, RelationGetRelid(index), true);
=20
/* Remember info about rel before closing OldHeap */
@@ -958,20 +961,20 @@ copy_table_data(Relation NewHeap, Relation OldHeap, R=
elation OldIndex, bool verb
/* Log what we're doing */
if (OldIndex !=3D NULL && !use_sort)
ereport(elevel,
- (errmsg("clustering \"%s.%s\" using index scan on \"%s\"",
- nspname,
- RelationGetRelationName(OldHeap),
- RelationGetRelationName(OldIndex))));
+ errmsg("repacking \"%s.%s\" using index scan on \"%s\"",
+ nspname,
+ RelationGetRelationName(OldHeap),
+ RelationGetRelationName(OldIndex)));
else if (use_sort)
ereport(elevel,
- (errmsg("clustering \"%s.%s\" using sequential scan and sort",
- nspname,
- RelationGetRelationName(OldHeap))));
+ errmsg("repacking \"%s.%s\" using sequential scan and sort",
+ nspname,
+ RelationGetRelationName(OldHeap)));
else
ereport(elevel,
- (errmsg("vacuuming \"%s.%s\"",
- nspname,
- RelationGetRelationName(OldHeap))));
+ errmsg("repacking \"%s.%s\" in physical order",
+ nspname,
+ RelationGetRelationName(OldHeap)));
=20
/*
* Hand off the actual copying to AM specific function, the generic code
@@ -1458,8 +1461,8 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
int i;
=20
/* Report that we are now swapping relation files */
- pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
- PROGRESS_CLUSTER_PHASE_SWAP_REL_FILES);
+ pgstat_progress_update_param(PROGRESS_REPACK_PHASE,
+ PROGRESS_REPACK_PHASE_SWAP_REL_FILES);
=20
/* Zero out possible results from swapped_relation_files */
memset(mapped_tables, 0, sizeof(mapped_tables));
@@ -1509,14 +1512,14 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
reindex_flags |=3D REINDEX_REL_FORCE_INDEXES_PERMANENT;
=20
/* Report that we are now reindexing relations */
- pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
- PROGRESS_CLUSTER_PHASE_REBUILD_INDEX);
+ pgstat_progress_update_param(PROGRESS_REPACK_PHASE,
+ PROGRESS_REPACK_PHASE_REBUILD_INDEX);
=20
reindex_relation(NULL, OIDOldHeap, reindex_flags, &reindex_params);
=20
/* Report that we are now doing clean up */
- pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
- PROGRESS_CLUSTER_PHASE_FINAL_CLEANUP);
+ pgstat_progress_update_param(PROGRESS_REPACK_PHASE,
+ PROGRESS_REPACK_PHASE_FINAL_CLEANUP);
=20
/*
* If the relation being rebuilt is pg_class, swap_relation_files()
@@ -1632,106 +1635,191 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
}
}
=20
-
/*
- * Get a list of tables that the current user has privileges on and
- * have indisclustered set. Return the list in a List * of RelToCluster
- * (stored in the specified memory context), each one giving the tableOid
- * and the indexOid on which the table is already clustered.
+ * Determine which relations to process, when REPACK/CLUSTER is called
+ * without specifying a table name. The exact process depends on whether
+ * USING INDEX was given or not, and in any case we only return tables and
+ * materialized views that the current user has privileges to repack/clust=
er.
+ *
+ * If USING INDEX was given, we scan pg_index to find those that have
+ * indisclustered set; if it was not given, scan pg_class and return all
+ * tables.
+ *
+ * Return it as a list of RelToCluster in the given memory context.
*/
static List *
-get_tables_to_cluster(MemoryContext cluster_context)
+get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext per=
mcxt)
{
- Relation indRelation;
+ Relation catalog;
TableScanDesc scan;
- ScanKeyData entry;
- HeapTuple indexTuple;
- Form_pg_index index;
- MemoryContext old_context;
+ HeapTuple tuple;
List *rtcs =3D NIL;
=20
- /*
- * Get all indexes that have indisclustered set and that the current user
- * has the appropriate privileges for.
- */
- indRelation =3D table_open(IndexRelationId, AccessShareLock);
- ScanKeyInit(&entry,
- Anum_pg_index_indisclustered,
- BTEqualStrategyNumber, F_BOOLEQ,
- BoolGetDatum(true));
- scan =3D table_beginscan_catalog(indRelation, 1, &entry);
- while ((indexTuple =3D heap_getnext(scan, ForwardScanDirection)) !=3D NUL=
L)
+ if (usingindex)
{
- RelToCluster *rtc;
+ ScanKeyData entry;
+
+ catalog =3D table_open(IndexRelationId, AccessShareLock);
+ ScanKeyInit(&entry,
+ Anum_pg_index_indisclustered,
+ BTEqualStrategyNumber, F_BOOLEQ,
+ BoolGetDatum(true));
+ scan =3D table_beginscan_catalog(catalog, 1, &entry);
+ while ((tuple =3D heap_getnext(scan, ForwardScanDirection)) !=3D NULL)
+ {
+ RelToCluster *rtc;
+ Form_pg_index index;
+ MemoryContext oldcxt;
=20
- index =3D (Form_pg_index) GETSTRUCT(indexTuple);
+ index =3D (Form_pg_index) GETSTRUCT(tuple);
=20
- if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId()))
- continue;
+ /*
+ * Try to obtain a light lock on the index's table, to ensure it
+ * doesn't go away while we collect the list. If we cannot, just
+ * disregard it.
+ */
+ if (!ConditionalLockRelationOid(index->indrelid, AccessShareLock))
+ continue;
=20
- /* Use a permanent memory context for the result list */
- old_context =3D MemoryContextSwitchTo(cluster_context);
+ /* Verify that the table still exists */
+ if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(index->indrelid)))
+ {
+ /* Release useless lock */
+ UnlockRelationOid(index->indrelid, AccessShareLock);
+ continue;
+ }
=20
- rtc =3D palloc_object(RelToCluster);
- rtc->tableOid =3D index->indrelid;
- rtc->indexOid =3D index->indexrelid;
- rtcs =3D lappend(rtcs, rtc);
+ if (!cluster_is_permitted_for_relation(cmd, index->indrelid,
+ GetUserId()))
+ continue;
+
+ /* Use a permanent memory context for the result list */
+ oldcxt =3D MemoryContextSwitchTo(permcxt);
+ rtc =3D palloc_object(RelToCluster);
+ rtc->tableOid =3D index->indrelid;
+ rtc->indexOid =3D index->indexrelid;
+ rtcs =3D lappend(rtcs, rtc);
+ MemoryContextSwitchTo(oldcxt);
+ }
+ }
+ else
+ {
+ catalog =3D table_open(RelationRelationId, AccessShareLock);
+ scan =3D table_beginscan_catalog(catalog, 0, NULL);
+
+ while ((tuple =3D heap_getnext(scan, ForwardScanDirection)) !=3D NULL)
+ {
+ RelToCluster *rtc;
+ Form_pg_class class;
+ MemoryContext oldcxt;
+
+ class =3D (Form_pg_class) GETSTRUCT(tuple);
+
+ /*
+ * Try to obtain a light lock on the table, to ensure it doesn't
+ * go away while we collect the list. If we cannot, just
+ * disregard the table.
+ */
+ if (!ConditionalLockRelationOid(class->oid, AccessShareLock))
+ continue;
=20
- MemoryContextSwitchTo(old_context);
+ /* Verify that the table still exists */
+ if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(class->oid)))
+ {
+ /* Release useless lock */
+ UnlockRelationOid(class->oid, AccessShareLock);
+ continue;
+ }
+
+ /* Can only process plain tables and matviews */
+ if (class->relkind !=3D RELKIND_RELATION &&
+ class->relkind !=3D RELKIND_MATVIEW)
+ continue;
+
+ /* noisily skip rels which the user can't process */
+ if (!cluster_is_permitted_for_relation(cmd, class->oid,
+ GetUserId()))
+ continue;
+
+ /* Use a permanent memory context for the result list */
+ oldcxt =3D MemoryContextSwitchTo(permcxt);
+ rtc =3D palloc_object(RelToCluster);
+ rtc->tableOid =3D class->oid;
+ rtc->indexOid =3D InvalidOid;
+ rtcs =3D lappend(rtcs, rtc);
+ MemoryContextSwitchTo(oldcxt);
+ }
}
- table_endscan(scan);
=20
- relation_close(indRelation, AccessShareLock);
+ table_endscan(scan);
+ relation_close(catalog, AccessShareLock);
=20
return rtcs;
}
=20
/*
- * Given an index on a partitioned table, return a list of RelToCluster for
+ * Given a partitioned table or its index, return a list of RelToCluster f=
or
* all the children leaves tables/indexes.
*
* Like expand_vacuum_rel, but here caller must hold AccessExclusiveLock
* on the table containing the index.
+ *
+ * 'rel_is_index' tells whether 'relid' is that of an index (true) or of t=
he
+ * owning relation.
*/
static List *
-get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid index=
Oid)
+get_tables_to_repack_partitioned(RepackCommand cmd, Oid relid,
+ bool rel_is_index, MemoryContext permcxt)
{
List *inhoids;
- ListCell *lc;
List *rtcs =3D NIL;
- MemoryContext old_context;
-
- /* Do not lock the children until they're processed */
- inhoids =3D find_all_inheritors(indexOid, NoLock, NULL);
=20
- foreach(lc, inhoids)
+ /*
+ * Do not lock the children until they're processed. Note that we do hold
+ * a lock on the parent partitioned table.
+ */
+ inhoids =3D find_all_inheritors(relid, NoLock, NULL);
+ foreach_oid(child_oid, inhoids)
{
- Oid indexrelid =3D lfirst_oid(lc);
- Oid relid =3D IndexGetRelation(indexrelid, false);
+ Oid table_oid,
+ index_oid;
RelToCluster *rtc;
+ MemoryContext oldcxt;
=20
- /* consider only leaf indexes */
- if (get_rel_relkind(indexrelid) !=3D RELKIND_INDEX)
- continue;
+ if (rel_is_index)
+ {
+ /* consider only leaf indexes */
+ if (get_rel_relkind(child_oid) !=3D RELKIND_INDEX)
+ continue;
+
+ table_oid =3D IndexGetRelation(child_oid, false);
+ index_oid =3D child_oid;
+ }
+ else
+ {
+ /* consider only leaf relations */
+ if (get_rel_relkind(child_oid) !=3D RELKIND_RELATION)
+ continue;
+
+ table_oid =3D child_oid;
+ index_oid =3D InvalidOid;
+ }
=20
/*
* It's possible that the user does not have privileges to CLUSTER the
- * leaf partition despite having such privileges on the partitioned
- * table. We skip any partitions which the user is not permitted to
- * CLUSTER.
+ * leaf partition despite having them on the partitioned table. Skip
+ * if so.
*/
- if (!cluster_is_permitted_for_relation(relid, GetUserId()))
+ if (!cluster_is_permitted_for_relation(cmd, table_oid, GetUserId()))
continue;
=20
/* Use a permanent memory context for the result list */
- old_context =3D MemoryContextSwitchTo(cluster_context);
-
+ oldcxt =3D MemoryContextSwitchTo(permcxt);
rtc =3D palloc_object(RelToCluster);
- rtc->tableOid =3D relid;
- rtc->indexOid =3D indexrelid;
+ rtc->tableOid =3D table_oid;
+ rtc->indexOid =3D index_oid;
rtcs =3D lappend(rtcs, rtc);
-
- MemoryContextSwitchTo(old_context);
+ MemoryContextSwitchTo(oldcxt);
}
=20
return rtcs;
@@ -1742,13 +1830,167 @@ get_tables_to_cluster_partitioned(MemoryContext cl=
uster_context, Oid indexOid)
* function emits a WARNING.
*/
static bool
-cluster_is_permitted_for_relation(Oid relid, Oid userid)
+cluster_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid)
{
+ Assert(cmd =3D=3D REPACK_COMMAND_CLUSTER || cmd =3D=3D REPACK_COMMAND_REP=
ACK);
+
if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) =3D=3D ACLCHECK_OK)
return true;
=20
ereport(WARNING,
- (errmsg("permission denied to cluster \"%s\", skipping it",
- get_rel_name(relid))));
+ errmsg("permission denied to execute %s on \"%s\", skipping it",
+ RepackCommandAsString(cmd),
+ get_rel_name(relid)));
+
return false;
}
+
+
+/*
+ * Given a RepackStmt with an indicated relation name, resolve the relation
+ * name, obtain lock on it, then determine what to do based on the relation
+ * type: if it's table and not partitioned, repack it as indicated (using =
an
+ * existing clustered index, or following the given one), and return NULL.
+ *
+ * On the other hand, if the table is partitioned, do nothing further and
+ * instead return the opened and locked relcache entry, so that caller can
+ * process the partitions using the multiple-table handling code. In this
+ * case, if an index name is given, it's up to the caller to resolve it.
+ */
+static Relation
+process_single_relation(RepackStmt *stmt, ClusterParams *params)
+{
+ Relation rel;
+ Oid tableOid;
+
+ Assert(stmt->relation !=3D NULL);
+ Assert(stmt->command =3D=3D REPACK_COMMAND_CLUSTER ||
+ stmt->command =3D=3D REPACK_COMMAND_REPACK);
+
+ /*
+ * Find, lock, and check permissions on the table. We obtain
+ * AccessExclusiveLock right away to avoid lock-upgrade hazard in the
+ * single-transaction case.
+ */
+ tableOid =3D RangeVarGetRelidExtended(stmt->relation->relation,
+ AccessExclusiveLock,
+ 0,
+ RangeVarCallbackMaintainsTable,
+ NULL);
+ rel =3D table_open(tableOid, NoLock);
+
+ /*
+ * Reject clustering a remote temp table ... their local buffer manager is
+ * not going to cope.
+ */
+ if (RELATION_IS_OTHER_TEMP(rel))
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot execute %s on temporary tables of other sessions",
+ RepackCommandAsString(stmt->command)));
+
+ /*
+ * Make sure ANALYZE is specified if a column list is present.
+ */
+ if ((params->options & CLUOPT_ANALYZE) =3D=3D 0 && stmt->relation->va_col=
s !=3D NIL)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ANALYZE option must be specified when a column list is provide=
d"));
+
+ /*
+ * For partitioned tables, let caller handle this. Otherwise, process it
+ * here and we're done.
+ */
+ if (rel->rd_rel->relkind =3D=3D RELKIND_PARTITIONED_TABLE)
+ return rel;
+ else
+ {
+ Oid indexOid;
+
+ indexOid =3D determine_clustered_index(rel, stmt->usingindex,
+ stmt->indexname);
+ if (OidIsValid(indexOid))
+ check_index_is_clusterable(rel, indexOid, AccessExclusiveLock);
+ cluster_rel(stmt->command, rel, indexOid, params);
+
+ /* Do an analyze, if requested */
+ if (params->options & CLUOPT_ANALYZE)
+ {
+ VacuumParams vac_params =3D {0};
+
+ vac_params.options |=3D VACOPT_ANALYZE;
+ if (params->options & CLUOPT_VERBOSE)
+ vac_params.options |=3D VACOPT_VERBOSE;
+ analyze_rel(tableOid, NULL, vac_params,
+ stmt->relation->va_cols, true, NULL);
+ }
+
+ return NULL;
+ }
+}
+
+/*
+ * Given a relation and the usingindex/indexname options in a
+ * REPACK USING INDEX or CLUSTER command, return the OID of the
+ * index to use for clustering the table.
+ *
+ * Caller must hold lock on the relation so that the set of indexes
+ * doesn't change, and must call check_index_is_clusterable.
+ */
+static Oid
+determine_clustered_index(Relation rel, bool usingindex, const char *index=
name)
+{
+ Oid indexOid;
+
+ if (indexname =3D=3D NULL && usingindex)
+ {
+ /*
+ * If USING INDEX with no name is given, find a clustered index, or
+ * error out if none.
+ */
+ indexOid =3D InvalidOid;
+ foreach_oid(idxoid, RelationGetIndexList(rel))
+ {
+ if (get_index_isclustered(idxoid))
+ {
+ indexOid =3D idxoid;
+ break;
+ }
+ }
+
+ if (!OidIsValid(indexOid))
+ ereport(ERROR,
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("there is no previously clustered index for table \"%s\"",
+ RelationGetRelationName(rel)));
+ }
+ else if (indexname !=3D NULL)
+ {
+ /* An index was specified; obtain its OID. */
+ indexOid =3D get_relname_relid(indexname, rel->rd_rel->relnamespace);
+ if (!OidIsValid(indexOid))
+ ereport(ERROR,
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("index \"%s\" for table \"%s\" does not exist",
+ indexname, RelationGetRelationName(rel)));
+ }
+ else
+ indexOid =3D InvalidOid;
+
+ return indexOid;
+}
+
+static const char *
+RepackCommandAsString(RepackCommand cmd)
+{
+ switch (cmd)
+ {
+ case REPACK_COMMAND_REPACK:
+ return "REPACK";
+ case REPACK_COMMAND_VACUUMFULL:
+ return "VACUUM";
+ case REPACK_COMMAND_CLUSTER:
+ return "CLUSTER";
+ }
+ return "???"; /* keep compiler quiet */
+}
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 03932f45c8a..aea998260e1 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -351,7 +351,6 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, boo=
l isTopLevel)
}
}
=20
-
/*
* Sanity check DISABLE_PAGE_SKIPPING option.
*/
@@ -2289,8 +2288,9 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParam=
s params,
if ((params.options & VACOPT_VERBOSE) !=3D 0)
cluster_params.options |=3D CLUOPT_VERBOSE;
=20
- /* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
- cluster_rel(rel, InvalidOid, &cluster_params);
+ /* VACUUM FULL is a variant of REPACK; see cluster.c */
+ cluster_rel(REPACK_COMMAND_VACUUMFULL, rel, InvalidOid,
+ &cluster_params);
/* cluster_rel closes the relation, but keeps lock */
=20
rel =3D NULL;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index c567252acc4..86f043523ac 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -287,7 +287,7 @@ static Node *makeRecursiveViewSelect(char *relname, Lis=
t *aliases, Node *query);
AlterCompositeTypeStmt AlterUserMappingStmt
AlterRoleStmt AlterRoleSetStmt AlterPolicyStmt AlterStatsStmt
AlterDefaultPrivilegesStmt DefACLAction
- AnalyzeStmt CallStmt ClosePortalStmt ClusterStmt CommentStmt
+ AnalyzeStmt CallStmt ClosePortalStmt CommentStmt
ConstraintsSetStmt CopyStmt CreateAsStmt CreateCastStmt
CreateDomainStmt CreateExtensionStmt CreateGroupStmt CreateOpClassStmt
CreateOpFamilyStmt AlterOpFamilyStmt CreatePLangStmt
@@ -304,7 +304,7 @@ static Node *makeRecursiveViewSelect(char *relname, Lis=
t *aliases, Node *query);
GrantStmt GrantRoleStmt ImportForeignSchemaStmt IndexStmt InsertStmt
ListenStmt LoadStmt LockStmt MergeStmt NotifyStmt ExplainableStmt Prepar=
ableStmt
CreateFunctionStmt AlterFunctionStmt ReindexStmt RemoveAggrStmt
- RemoveFuncStmt RemoveOperStmt RenameStmt ReturnStmt RevokeStmt RevokeRol=
eStmt
+ RemoveFuncStmt RemoveOperStmt RenameStmt RepackStmt ReturnStmt RevokeStm=
t RevokeRoleStmt
RuleActionStmt RuleActionStmtOrEmpty RuleStmt
SecLabelStmt SelectStmt TransactionStmt TransactionStmtLegacy TruncateSt=
mt
UnlistenStmt UpdateStmt VacuumStmt
@@ -323,7 +323,7 @@ static Node *makeRecursiveViewSelect(char *relname, Lis=
t *aliases, Node *query);
=20
%type <str> opt_single_name
%type <list> opt_qualified_name
-%type <boolean> opt_concurrently
+%type <boolean> opt_concurrently opt_usingindex
%type <dbehavior> opt_drop_behavior
%type <list> opt_utility_option_list
%type <list> opt_wait_with_clause
@@ -773,7 +773,7 @@ static Node *makeRecursiveViewSelect(char *relname, Lis=
t *aliases, Node *query);
QUOTE QUOTES
=20
RANGE READ REAL REASSIGN RECURSIVE REF_P REFERENCES REFERENCING
- REFRESH REINDEX RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
+ REFRESH REINDEX RELATIVE_P RELEASE RENAME REPACK REPEATABLE REPLACE REPLI=
CA
RESET RESPECT_P RESTART RESTRICT RETURN RETURNING RETURNS REVOKE RIGHT RO=
LE ROLLBACK ROLLUP
ROUTINE ROUTINES ROW ROWS RULE
=20
@@ -1035,7 +1035,6 @@ stmt:
| CallStmt
| CheckPointStmt
| ClosePortalStmt
- | ClusterStmt
| CommentStmt
| ConstraintsSetStmt
| CopyStmt
@@ -1109,6 +1108,7 @@ stmt:
| RemoveFuncStmt
| RemoveOperStmt
| RenameStmt
+ | RepackStmt
| RevokeStmt
| RevokeRoleStmt
| RuleStmt
@@ -1146,6 +1146,11 @@ opt_concurrently:
| /*EMPTY*/ { $$ =3D false; }
;
=20
+opt_usingindex:
+ USING INDEX { $$ =3D true; }
+ | /* EMPTY */ { $$ =3D false; }
+ ;
+
opt_drop_behavior:
CASCADE { $$ =3D DROP_CASCADE; }
| RESTRICT { $$ =3D DROP_RESTRICT; }
@@ -12036,38 +12041,82 @@ CreateConversionStmt:
/*************************************************************************=
****
*
* QUERY:
+ * REPACK [ (options) ] [ <qualified_name> [ <name_list> ] [ USING INDE=
X <index_name> ] ]
+ *
+ * obsolete variants:
* CLUSTER (options) [ <qualified_name> [ USING <index_name> ] ]
* CLUSTER [VERBOSE] [ <qualified_name> [ USING <index_name> ] ]
* CLUSTER [VERBOSE] <index_name> ON <qualified_name> (for pre-8.3)
*
*************************************************************************=
****/
=20
-ClusterStmt:
- CLUSTER '(' utility_option_list ')' qualified_name cluster_index_specif=
ication
+RepackStmt:
+ REPACK opt_utility_option_list vacuum_relation USING INDEX name
{
- ClusterStmt *n =3D makeNode(ClusterStmt);
+ RepackStmt *n =3D makeNode(RepackStmt);
=20
- n->relation =3D $5;
+ n->command =3D REPACK_COMMAND_REPACK;
+ n->relation =3D (VacuumRelation *) $3;
n->indexname =3D $6;
+ n->usingindex =3D true;
+ n->params =3D $2;
+ $$ =3D (Node *) n;
+ }
+ | REPACK opt_utility_option_list vacuum_relation opt_usingindex
+ {
+ RepackStmt *n =3D makeNode(RepackStmt);
+
+ n->command =3D REPACK_COMMAND_REPACK;
+ n->relation =3D (VacuumRelation *) $3;
+ n->indexname =3D NULL;
+ n->usingindex =3D $4;
+ n->params =3D $2;
+ $$ =3D (Node *) n;
+ }
+ | REPACK opt_utility_option_list opt_usingindex
+ {
+ RepackStmt *n =3D makeNode(RepackStmt);
+
+ n->command =3D REPACK_COMMAND_REPACK;
+ n->relation =3D NULL;
+ n->indexname =3D NULL;
+ n->usingindex =3D $3;
+ n->params =3D $2;
+ $$ =3D (Node *) n;
+ }
+ | CLUSTER '(' utility_option_list ')' qualified_name cluster_index_spec=
ification
+ {
+ RepackStmt *n =3D makeNode(RepackStmt);
+
+ n->command =3D REPACK_COMMAND_CLUSTER;
+ n->relation =3D makeNode(VacuumRelation);
+ n->relation->relation =3D $5;
+ n->indexname =3D $6;
+ n->usingindex =3D true;
n->params =3D $3;
$$ =3D (Node *) n;
}
| CLUSTER opt_utility_option_list
{
- ClusterStmt *n =3D makeNode(ClusterStmt);
+ RepackStmt *n =3D makeNode(RepackStmt);
=20
+ n->command =3D REPACK_COMMAND_CLUSTER;
n->relation =3D NULL;
n->indexname =3D NULL;
+ n->usingindex =3D true;
n->params =3D $2;
$$ =3D (Node *) n;
}
/* unparenthesized VERBOSE kept for pre-14 compatibility */
| CLUSTER opt_verbose qualified_name cluster_index_specification
{
- ClusterStmt *n =3D makeNode(ClusterStmt);
+ RepackStmt *n =3D makeNode(RepackStmt);
=20
- n->relation =3D $3;
+ n->command =3D REPACK_COMMAND_CLUSTER;
+ n->relation =3D makeNode(VacuumRelation);
+ n->relation->relation =3D $3;
n->indexname =3D $4;
+ n->usingindex =3D true;
if ($2)
n->params =3D list_make1(makeDefElem("verbose", NULL, @2));
$$ =3D (Node *) n;
@@ -12075,20 +12124,25 @@ ClusterStmt:
/* unparenthesized VERBOSE kept for pre-17 compatibility */
| CLUSTER VERBOSE
{
- ClusterStmt *n =3D makeNode(ClusterStmt);
+ RepackStmt *n =3D makeNode(RepackStmt);
=20
+ n->command =3D REPACK_COMMAND_CLUSTER;
n->relation =3D NULL;
n->indexname =3D NULL;
+ n->usingindex =3D true;
n->params =3D list_make1(makeDefElem("verbose", NULL, @2));
$$ =3D (Node *) n;
}
/* kept for pre-8.3 compatibility */
| CLUSTER opt_verbose name ON qualified_name
{
- ClusterStmt *n =3D makeNode(ClusterStmt);
+ RepackStmt *n =3D makeNode(RepackStmt);
=20
- n->relation =3D $5;
+ n->command =3D REPACK_COMMAND_CLUSTER;
+ n->relation =3D makeNode(VacuumRelation);
+ n->relation->relation =3D $5;
n->indexname =3D $3;
+ n->usingindex =3D true;
if ($2)
n->params =3D list_make1(makeDefElem("verbose", NULL, @2));
$$ =3D (Node *) n;
@@ -18145,6 +18199,7 @@ unreserved_keyword:
| RELATIVE_P
| RELEASE
| RENAME
+ | REPACK
| REPEATABLE
| REPLACE
| REPLICA
@@ -18782,6 +18837,7 @@ bare_label_keyword:
| RELATIVE_P
| RELEASE
| RENAME
+ | REPACK
| REPEATABLE
| REPLACE
| REPLICA
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 34dd6e18df5..ca737b05115 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -279,9 +279,9 @@ ClassifyUtilityCommandAsReadOnly(Node *parsetree)
return COMMAND_OK_IN_RECOVERY | COMMAND_OK_IN_READ_ONLY_TXN;
}
=20
- case T_ClusterStmt:
case T_ReindexStmt:
case T_VacuumStmt:
+ case T_RepackStmt:
{
/*
* These commands write WAL, so they're not strictly
@@ -856,14 +856,14 @@ standard_ProcessUtility(PlannedStmt *pstmt,
ExecuteCallStmt(castNode(CallStmt, parsetree), params, isAtomicContext,=
dest);
break;
=20
- case T_ClusterStmt:
- cluster(pstate, (ClusterStmt *) parsetree, isTopLevel);
- break;
-
case T_VacuumStmt:
ExecVacuum(pstate, (VacuumStmt *) parsetree, isTopLevel);
break;
=20
+ case T_RepackStmt:
+ ExecRepack(pstate, (RepackStmt *) parsetree, isTopLevel);
+ break;
+
case T_ExplainStmt:
ExplainQuery(pstate, (ExplainStmt *) parsetree, params, dest);
break;
@@ -2865,10 +2865,6 @@ CreateCommandTag(Node *parsetree)
tag =3D CMDTAG_CALL;
break;
=20
- case T_ClusterStmt:
- tag =3D CMDTAG_CLUSTER;
- break;
-
case T_VacuumStmt:
if (((VacuumStmt *) parsetree)->is_vacuumcmd)
tag =3D CMDTAG_VACUUM;
@@ -2876,6 +2872,13 @@ CreateCommandTag(Node *parsetree)
tag =3D CMDTAG_ANALYZE;
break;
=20
+ case T_RepackStmt:
+ if (((RepackStmt *) parsetree)->command =3D=3D REPACK_COMMAND_CLUSTER)
+ tag =3D CMDTAG_CLUSTER;
+ else
+ tag =3D CMDTAG_REPACK;
+ break;
+
case T_ExplainStmt:
tag =3D CMDTAG_EXPLAIN;
break;
@@ -3517,7 +3520,7 @@ GetCommandLogLevel(Node *parsetree)
lev =3D LOGSTMT_ALL;
break;
=20
- case T_ClusterStmt:
+ case T_RepackStmt:
lev =3D LOGSTMT_DDL;
break;
=20
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pg=
statfuncs.c
index b1df96e7b0b..15accbce2ba 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -287,8 +287,8 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
cmdtype =3D PROGRESS_COMMAND_VACUUM;
else if (pg_strcasecmp(cmd, "ANALYZE") =3D=3D 0)
cmdtype =3D PROGRESS_COMMAND_ANALYZE;
- else if (pg_strcasecmp(cmd, "CLUSTER") =3D=3D 0)
- cmdtype =3D PROGRESS_COMMAND_CLUSTER;
+ else if (pg_strcasecmp(cmd, "REPACK") =3D=3D 0)
+ cmdtype =3D PROGRESS_COMMAND_REPACK;
else if (pg_strcasecmp(cmd, "CREATE INDEX") =3D=3D 0)
cmdtype =3D PROGRESS_COMMAND_CREATE_INDEX;
else if (pg_strcasecmp(cmd, "BASEBACKUP") =3D=3D 0)
diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 8b91bc00062..2a1bb47ff03 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -1267,7 +1267,7 @@ static const char *const sql_commands[] =3D {
"DELETE FROM", "DISCARD", "DO", "DROP", "END", "EXECUTE", "EXPLAIN",
"FETCH", "GRANT", "IMPORT FOREIGN SCHEMA", "INSERT INTO", "LISTEN", "LOAD=
", "LOCK",
"MERGE INTO", "MOVE", "NOTIFY", "PREPARE",
- "REASSIGN", "REFRESH MATERIALIZED VIEW", "REINDEX", "RELEASE",
+ "REASSIGN", "REFRESH MATERIALIZED VIEW", "REINDEX", "RELEASE", "REPACK",
"RESET", "REVOKE", "ROLLBACK",
"SAVEPOINT", "SECURITY LABEL", "SELECT", "SET", "SHOW", "START",
"TABLE", "TRUNCATE", "UNLISTEN", "UPDATE", "VACUUM", "VALUES",
@@ -5086,6 +5086,46 @@ match_previous_words(int pattern_id,
COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
}
=20
+/* REPACK */
+ else if (Matches("REPACK"))
+ COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_clusterables,
+ "(", "USING INDEX");
+ else if (Matches("REPACK", "(*)"))
+ COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_clusterables,
+ "USING INDEX");
+ else if (Matches("REPACK", MatchAnyExcept("(")))
+ COMPLETE_WITH("USING INDEX");
+ else if (Matches("REPACK", "(*)", MatchAnyExcept("(")))
+ COMPLETE_WITH("USING INDEX");
+ else if (Matches("REPACK", MatchAny, "USING", "INDEX") ||
+ Matches("REPACK", "(*)", MatchAny, "USING", "INDEX"))
+ {
+ set_completion_reference(prev3_wd);
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_index_of_table);
+ }
+ /*
+ * Complete ... [ (*) ] <sth> USING INDEX, with a list of indexes for
+ * <sth>.
+ */
+ else if (TailMatches(MatchAny, "USING", "INDEX"))
+ {
+ set_completion_reference(prev3_wd);
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_index_of_table);
+ }
+ else if (HeadMatches("REPACK", "(*") &&
+ !HeadMatches("REPACK", "(*)"))
+ {
+ /*
+ * This fires if we're in an unfinished parenthesized option list.
+ * get_previous_words treats a completed parenthesized option list as
+ * one word, so the above test is correct.
+ */
+ if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
+ COMPLETE_WITH("ANALYZE", "VERBOSE");
+ else if (TailMatches("ANALYZE", "VERBOSE"))
+ COMPLETE_WITH("ON", "OFF");
+ }
+
/* SECURITY LABEL */
else if (Matches("SECURITY"))
COMPLETE_WITH("LABEL");
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 8ea81622f9d..28741988478 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -24,6 +24,7 @@
#define CLUOPT_RECHECK 0x02 /* recheck relation state */
#define CLUOPT_RECHECK_ISCLUSTERED 0x04 /* recheck relation state for
* indisclustered */
+#define CLUOPT_ANALYZE 0x08 /* do an ANALYZE */
=20
/* options for CLUSTER */
typedef struct ClusterParams
@@ -31,8 +32,11 @@ typedef struct ClusterParams
bits32 options; /* bitmask of CLUOPT_* */
} ClusterParams;
=20
-extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel=
);
-extern void cluster_rel(Relation OldHeap, Oid indexOid, ClusterParams *par=
ams);
+
+extern void ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLev=
el);
+
+extern void cluster_rel(RepackCommand command, Relation OldHeap, Oid index=
Oid,
+ ClusterParams *params);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_inter=
nal);
diff --git a/src/include/commands/progress.h b/src/include/commands/progres=
s.h
index 359221dc296..f00e39b937d 100644
--- a/src/include/commands/progress.h
+++ b/src/include/commands/progress.h
@@ -73,28 +73,34 @@
#define PROGRESS_ANALYZE_STARTED_BY_MANUAL 1
#define PROGRESS_ANALYZE_STARTED_BY_AUTOVACUUM 2
=20
-/* Progress parameters for cluster */
-#define PROGRESS_CLUSTER_COMMAND 0
-#define PROGRESS_CLUSTER_PHASE 1
-#define PROGRESS_CLUSTER_INDEX_RELID 2
-#define PROGRESS_CLUSTER_HEAP_TUPLES_SCANNED 3
-#define PROGRESS_CLUSTER_HEAP_TUPLES_WRITTEN 4
-#define PROGRESS_CLUSTER_TOTAL_HEAP_BLKS 5
-#define PROGRESS_CLUSTER_HEAP_BLKS_SCANNED 6
-#define PROGRESS_CLUSTER_INDEX_REBUILD_COUNT 7
-
-/* Phases of cluster (as advertised via PROGRESS_CLUSTER_PHASE) */
-#define PROGRESS_CLUSTER_PHASE_SEQ_SCAN_HEAP 1
-#define PROGRESS_CLUSTER_PHASE_INDEX_SCAN_HEAP 2
-#define PROGRESS_CLUSTER_PHASE_SORT_TUPLES 3
-#define PROGRESS_CLUSTER_PHASE_WRITE_NEW_HEAP 4
-#define PROGRESS_CLUSTER_PHASE_SWAP_REL_FILES 5
-#define PROGRESS_CLUSTER_PHASE_REBUILD_INDEX 6
-#define PROGRESS_CLUSTER_PHASE_FINAL_CLEANUP 7
-
-/* Commands of PROGRESS_CLUSTER */
-#define PROGRESS_CLUSTER_COMMAND_CLUSTER 1
-#define PROGRESS_CLUSTER_COMMAND_VACUUM_FULL 2
+/*
+ * Progress parameters for REPACK.
+ *
+ * Values for PROGRESS_REPACK_COMMAND are defined as in RepackCommand.
+ *
+ * Note: Since REPACK shares code with CLUSTER, these values are also
+ * used by CLUSTER. (CLUSTER is now deprecated, so it makes little sense to
+ * introduce a separate set of constants.)
+ */
+#define PROGRESS_REPACK_COMMAND 0
+#define PROGRESS_REPACK_PHASE 1
+#define PROGRESS_REPACK_INDEX_RELID 2
+#define PROGRESS_REPACK_HEAP_TUPLES_SCANNED 3
+#define PROGRESS_REPACK_HEAP_TUPLES_WRITTEN 4
+#define PROGRESS_REPACK_TOTAL_HEAP_BLKS 5
+#define PROGRESS_REPACK_HEAP_BLKS_SCANNED 6
+#define PROGRESS_REPACK_INDEX_REBUILD_COUNT 7
+
+/*
+ * Phases of repack (as advertised via PROGRESS_REPACK_PHASE).
+ */
+#define PROGRESS_REPACK_PHASE_SEQ_SCAN_HEAP 1
+#define PROGRESS_REPACK_PHASE_INDEX_SCAN_HEAP 2
+#define PROGRESS_REPACK_PHASE_SORT_TUPLES 3
+#define PROGRESS_REPACK_PHASE_WRITE_NEW_HEAP 4
+#define PROGRESS_REPACK_PHASE_SWAP_REL_FILES 5
+#define PROGRESS_REPACK_PHASE_REBUILD_INDEX 6
+#define PROGRESS_REPACK_PHASE_FINAL_CLEANUP 7
=20
/* Progress parameters for CREATE INDEX */
/* 3, 4 and 5 reserved for "waitfor" metrics */
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 0aec49bdd22..dac839cdf1e 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3982,18 +3982,6 @@ typedef struct AlterSystemStmt
VariableSetStmt *setstmt; /* SET subcommand */
} AlterSystemStmt;
=20
-/* ----------------------
- * Cluster Statement (support pbrown's cluster index implementation)
- * ----------------------
- */
-typedef struct ClusterStmt
-{
- NodeTag type;
- RangeVar *relation; /* relation being indexed, or NULL if all */
- char *indexname; /* original index defined */
- List *params; /* list of DefElem nodes */
-} ClusterStmt;
-
/* ----------------------
* Vacuum and Analyze Statements
*
@@ -4006,7 +3994,7 @@ typedef struct VacuumStmt
NodeTag type;
List *options; /* list of DefElem nodes */
List *rels; /* list of VacuumRelation, or NIL for all */
- bool is_vacuumcmd; /* true for VACUUM, false for ANALYZE */
+ bool is_vacuumcmd; /* true for VACUUM, false otherwise */
} VacuumStmt;
=20
/*
@@ -4024,6 +4012,27 @@ typedef struct VacuumRelation
List *va_cols; /* list of column names, or NIL for all */
} VacuumRelation;
=20
+/* ----------------------
+ * Repack Statement
+ * ----------------------
+ */
+typedef enum RepackCommand
+{
+ REPACK_COMMAND_CLUSTER =3D 1,
+ REPACK_COMMAND_REPACK,
+ REPACK_COMMAND_VACUUMFULL,
+} RepackCommand;
+
+typedef struct RepackStmt
+{
+ NodeTag type;
+ RepackCommand command; /* type of command being run */
+ VacuumRelation *relation; /* relation being repacked */
+ char *indexname; /* order tuples by this index */
+ bool usingindex; /* whether USING INDEX is specified */
+ List *params; /* list of DefElem nodes */
+} RepackStmt;
+
/* ----------------------
* Explain Statement
*
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index f7753c5c8a8..6f74a8c05c7 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -377,6 +377,7 @@ PG_KEYWORD("reindex", REINDEX, UNRESERVED_KEYWORD, BARE=
_LABEL)
PG_KEYWORD("relative", RELATIVE_P, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("release", RELEASE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("rename", RENAME, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("repack", REPACK, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("repeatable", REPEATABLE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("replace", REPLACE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("replica", REPLICA, UNRESERVED_KEYWORD, BARE_LABEL)
diff --git a/src/include/tcop/cmdtaglist.h b/src/include/tcop/cmdtaglist.h
index 1290c9bab68..652dc61b834 100644
--- a/src/include/tcop/cmdtaglist.h
+++ b/src/include/tcop/cmdtaglist.h
@@ -196,6 +196,7 @@ PG_CMDTAG(CMDTAG_REASSIGN_OWNED, "REASSIGN OWNED", fals=
e, false, false)
PG_CMDTAG(CMDTAG_REFRESH_MATERIALIZED_VIEW, "REFRESH MATERIALIZED VIEW", t=
rue, false, false)
PG_CMDTAG(CMDTAG_REINDEX, "REINDEX", true, false, false)
PG_CMDTAG(CMDTAG_RELEASE, "RELEASE", false, false, false)
+PG_CMDTAG(CMDTAG_REPACK, "REPACK", false, false, false)
PG_CMDTAG(CMDTAG_RESET, "RESET", false, false, false)
PG_CMDTAG(CMDTAG_REVOKE, "REVOKE", true, false, false)
PG_CMDTAG(CMDTAG_REVOKE_ROLE, "REVOKE ROLE", false, false, false)
diff --git a/src/include/utils/backend_progress.h b/src/include/utils/backe=
nd_progress.h
index 19f63b41431..6300dbd15d5 100644
--- a/src/include/utils/backend_progress.h
+++ b/src/include/utils/backend_progress.h
@@ -24,10 +24,10 @@ typedef enum ProgressCommandType
PROGRESS_COMMAND_INVALID,
PROGRESS_COMMAND_VACUUM,
PROGRESS_COMMAND_ANALYZE,
- PROGRESS_COMMAND_CLUSTER,
PROGRESS_COMMAND_CREATE_INDEX,
PROGRESS_COMMAND_BASEBACKUP,
PROGRESS_COMMAND_COPY,
+ PROGRESS_COMMAND_REPACK,
} ProgressCommandType;
=20
#define PGSTAT_NUM_PROGRESS_PARAM 20
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expec=
ted/cluster.out
index 4d40a6809ab..277854418fa 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -495,6 +495,43 @@ ALTER TABLE clstrpart SET WITHOUT CLUSTER;
ERROR: cannot mark index clustered in partitioned table
ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
ERROR: cannot mark index clustered in partitioned table
+-- Check that REPACK sets new relfilenodes: it should process exactly the =
same
+-- tables as CLUSTER did.
+DROP TABLE old_cluster_info;
+DROP TABLE new_cluster_info;
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, =
relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class=
c ON c.oid=3Dtree.relid ;
+REPACK clstrpart USING INDEX clstrpart_idx;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, =
relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class=
c ON c.oid=3Dtree.relid ;
+SELECT relname, old.level, old.relkind, old.relfilenode =3D new.relfilenod=
e FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname)=
ORDER BY relname COLLATE "C";
+ relname | level | relkind | ?column?=20
+-------------+-------+---------+----------
+ clstrpart | 0 | p | t
+ clstrpart1 | 1 | p | t
+ clstrpart11 | 2 | r | f
+ clstrpart12 | 2 | p | t
+ clstrpart2 | 1 | r | f
+ clstrpart3 | 1 | p | t
+ clstrpart33 | 2 | r | f
+(7 rows)
+
+-- And finally the same for REPACK w/o index.
+DROP TABLE old_cluster_info;
+DROP TABLE new_cluster_info;
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, =
relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class=
c ON c.oid=3Dtree.relid ;
+REPACK clstrpart;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, =
relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class=
c ON c.oid=3Dtree.relid ;
+SELECT relname, old.level, old.relkind, old.relfilenode =3D new.relfilenod=
e FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname)=
ORDER BY relname COLLATE "C";
+ relname | level | relkind | ?column?=20
+-------------+-------+---------+----------
+ clstrpart | 0 | p | t
+ clstrpart1 | 1 | p | t
+ clstrpart11 | 2 | r | f
+ clstrpart12 | 2 | p | t
+ clstrpart2 | 1 | r | f
+ clstrpart3 | 1 | p | t
+ clstrpart33 | 2 | r | f
+(7 rows)
+
DROP TABLE clstrpart;
-- Ownership of partitions is checked
CREATE TABLE ptnowner(i int unique) PARTITION BY LIST (i);
@@ -513,7 +550,7 @@ CREATE TEMP TABLE ptnowner_oldnodes AS
JOIN pg_class AS c ON c.oid=3Dtree.relid;
SET SESSION AUTHORIZATION regress_ptnowner;
CLUSTER ptnowner USING ptnowner_i_idx;
-WARNING: permission denied to cluster "ptnowner2", skipping it
+WARNING: permission denied to execute CLUSTER on "ptnowner2", skipping it
RESET SESSION AUTHORIZATION;
SELECT a.relname, a.relfilenode=3Db.relfilenode FROM pg_class a
JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C";
@@ -665,6 +702,101 @@ SELECT * FROM clstr_expression WHERE -a =3D -3 ORDER =
BY -a, b;
(4 rows)
=20
COMMIT;
+----------------------------------------------------------------------
+--
+-- REPACK
+--
+----------------------------------------------------------------------
+-- REPACK handles individual tables identically to CLUSTER, but it's worth
+-- checking if it handles table hierarchies identically as well.
+REPACK clstr_tst USING INDEX clstr_tst_c;
+-- Verify that inheritance link still works
+INSERT INTO clstr_tst_inh VALUES (0, 100, 'in child table 2');
+SELECT a,b,c,substring(d for 30), length(d) from clstr_tst;
+ a | b | c | substring | length=20
+----+-----+------------------+--------------------------------+--------
+ 10 | 14 | catorce | |=20=20=20=
=20=20=20=20
+ 18 | 5 | cinco | |=20=20=20=
=20=20=20=20
+ 9 | 4 | cuatro | |=20=20=20=
=20=20=20=20
+ 26 | 19 | diecinueve | |=20=20=20=
=20=20=20=20
+ 12 | 18 | dieciocho | |=20=20=20=
=20=20=20=20
+ 30 | 16 | dieciseis | |=20=20=20=
=20=20=20=20
+ 24 | 17 | diecisiete | |=20=20=20=
=20=20=20=20
+ 2 | 10 | diez | |=20=20=20=
=20=20=20=20
+ 23 | 12 | doce | |=20=20=20=
=20=20=20=20
+ 11 | 2 | dos | |=20=20=20=
=20=20=20=20
+ 25 | 9 | nueve | |=20=20=20=
=20=20=20=20
+ 31 | 8 | ocho | |=20=20=20=
=20=20=20=20
+ 1 | 11 | once | |=20=20=20=
=20=20=20=20
+ 28 | 15 | quince | |=20=20=20=
=20=20=20=20
+ 32 | 6 | seis | xyzzyxyzzyxyzzyxyzzyxyzzyxyzzy | 500000
+ 29 | 7 | siete | |=20=20=20=
=20=20=20=20
+ 15 | 13 | trece | |=20=20=20=
=20=20=20=20
+ 22 | 30 | treinta | |=20=20=20=
=20=20=20=20
+ 17 | 32 | treinta y dos | |=20=20=20=
=20=20=20=20
+ 3 | 31 | treinta y uno | |=20=20=20=
=20=20=20=20
+ 5 | 3 | tres | |=20=20=20=
=20=20=20=20
+ 20 | 1 | uno | |=20=20=20=
=20=20=20=20
+ 6 | 20 | veinte | |=20=20=20=
=20=20=20=20
+ 14 | 25 | veinticinco | |=20=20=20=
=20=20=20=20
+ 21 | 24 | veinticuatro | |=20=20=20=
=20=20=20=20
+ 4 | 22 | veintidos | |=20=20=20=
=20=20=20=20
+ 19 | 29 | veintinueve | |=20=20=20=
=20=20=20=20
+ 16 | 28 | veintiocho | |=20=20=20=
=20=20=20=20
+ 27 | 26 | veintiseis | |=20=20=20=
=20=20=20=20
+ 13 | 27 | veintisiete | |=20=20=20=
=20=20=20=20
+ 7 | 23 | veintitres | |=20=20=20=
=20=20=20=20
+ 8 | 21 | veintiuno | |=20=20=20=
=20=20=20=20
+ 0 | 100 | in child table | |=20=20=20=
=20=20=20=20
+ 0 | 100 | in child table 2 | |=20=20=20=
=20=20=20=20
+(34 rows)
+
+-- Verify that foreign key link still works
+INSERT INTO clstr_tst (b, c) VALUES (1111, 'this should fail');
+ERROR: insert or update on table "clstr_tst" violates foreign key constra=
int "clstr_tst_con"
+DETAIL: Key (b)=3D(1111) is not present in table "clstr_tst_s".
+SELECT conname FROM pg_constraint WHERE conrelid =3D 'clstr_tst'::regclass
+ORDER BY 1;
+ conname=20=20=20=20=20=20=20=20
+----------------------
+ clstr_tst_a_not_null
+ clstr_tst_con
+ clstr_tst_pkey
+(3 rows)
+
+-- Verify partial analyze works
+REPACK (ANALYZE) clstr_tst (a);
+REPACK (ANALYZE) clstr_tst;
+REPACK (VERBOSE) clstr_tst (a);
+ERROR: ANALYZE option must be specified when a column list is provided
+-- REPACK w/o argument performs no ordering, so we can only check which ta=
bles
+-- have the relfilenode changed.
+RESET SESSION AUTHORIZATION;
+CREATE TEMP TABLE relnodes_old AS
+(SELECT relname, relfilenode
+FROM pg_class
+WHERE relname IN ('clstr_1', 'clstr_2', 'clstr_3'));
+SET SESSION AUTHORIZATION regress_clstr_user;
+SET client_min_messages =3D ERROR; -- order of "skipping" warnings may va=
ry
+REPACK;
+RESET client_min_messages;
+RESET SESSION AUTHORIZATION;
+CREATE TEMP TABLE relnodes_new AS
+(SELECT relname, relfilenode
+FROM pg_class
+WHERE relname IN ('clstr_1', 'clstr_2', 'clstr_3'));
+-- Do the actual comparison. Unlike CLUSTER, clstr_3 should have been
+-- processed because there is nothing like clustering index here.
+SELECT o.relname FROM relnodes_old o
+JOIN relnodes_new n ON o.relname =3D n.relname
+WHERE o.relfilenode <> n.relfilenode
+ORDER BY o.relname;
+ relname=20
+---------
+ clstr_1
+ clstr_3
+(2 rows)
+
-- clean up
DROP TABLE clustertest;
DROP TABLE clstr_1;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expecte=
d/rules.out
index 78a37d9fc8f..6827166afac 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2002,34 +2002,23 @@ pg_stat_progress_basebackup| SELECT pid,
ELSE NULL::text
END AS backup_type
FROM pg_stat_get_progress_info('BASEBACKUP'::text) s(pid, datid, relid,=
param1, param2, param3, param4, param5, param6, param7, param8, param9, pa=
ram10, param11, param12, param13, param14, param15, param16, param17, param=
18, param19, param20);
-pg_stat_progress_cluster| SELECT s.pid,
- s.datid,
- d.datname,
- s.relid,
- CASE s.param1
- WHEN 1 THEN 'CLUSTER'::text
- WHEN 2 THEN 'VACUUM FULL'::text
- ELSE NULL::text
+pg_stat_progress_cluster| SELECT pid,
+ datid,
+ datname,
+ relid,
+ CASE
+ WHEN (command =3D ANY (ARRAY['CLUSTER'::text, 'VACUUM FULL'::t=
ext])) THEN command
+ WHEN (repack_index_relid =3D (0)::oid) THEN 'VACUUM FULL'::text
+ ELSE 'CLUSTER'::text
END AS command,
- CASE s.param2
- WHEN 0 THEN 'initializing'::text
- WHEN 1 THEN 'seq scanning heap'::text
- WHEN 2 THEN 'index scanning heap'::text
- WHEN 3 THEN 'sorting tuples'::text
- WHEN 4 THEN 'writing new heap'::text
- WHEN 5 THEN 'swapping relation files'::text
- WHEN 6 THEN 'rebuilding index'::text
- WHEN 7 THEN 'performing final cleanup'::text
- ELSE NULL::text
- END AS phase,
- (s.param3)::oid AS cluster_index_relid,
- s.param4 AS heap_tuples_scanned,
- s.param5 AS heap_tuples_written,
- s.param6 AS heap_blks_total,
- s.param7 AS heap_blks_scanned,
- s.param8 AS index_rebuild_count
- FROM (pg_stat_get_progress_info('CLUSTER'::text) s(pid, datid, relid, p=
aram1, param2, param3, param4, param5, param6, param7, param8, param9, para=
m10, param11, param12, param13, param14, param15, param16, param17, param18=
, param19, param20)
- LEFT JOIN pg_database d ON ((s.datid =3D d.oid)));
+ phase,
+ repack_index_relid AS cluster_index_relid,
+ heap_tuples_scanned,
+ heap_tuples_written,
+ heap_blks_total,
+ heap_blks_scanned,
+ index_rebuild_count
+ FROM pg_stat_progress_repack;
pg_stat_progress_copy| SELECT s.pid,
s.datid,
d.datname,
@@ -2089,6 +2078,35 @@ pg_stat_progress_create_index| SELECT s.pid,
s.param15 AS partitions_done
FROM (pg_stat_get_progress_info('CREATE INDEX'::text) s(pid, datid, rel=
id, param1, param2, param3, param4, param5, param6, param7, param8, param9,=
param10, param11, param12, param13, param14, param15, param16, param17, pa=
ram18, param19, param20)
LEFT JOIN pg_database d ON ((s.datid =3D d.oid)));
+pg_stat_progress_repack| SELECT s.pid,
+ s.datid,
+ d.datname,
+ s.relid,
+ CASE s.param1
+ WHEN 1 THEN 'CLUSTER'::text
+ WHEN 2 THEN 'REPACK'::text
+ WHEN 3 THEN 'VACUUM FULL'::text
+ ELSE NULL::text
+ END AS command,
+ CASE s.param2
+ WHEN 0 THEN 'initializing'::text
+ WHEN 1 THEN 'seq scanning heap'::text
+ WHEN 2 THEN 'index scanning heap'::text
+ WHEN 3 THEN 'sorting tuples'::text
+ WHEN 4 THEN 'writing new heap'::text
+ WHEN 5 THEN 'swapping relation files'::text
+ WHEN 6 THEN 'rebuilding index'::text
+ WHEN 7 THEN 'performing final cleanup'::text
+ ELSE NULL::text
+ END AS phase,
+ (s.param3)::oid AS repack_index_relid,
+ s.param4 AS heap_tuples_scanned,
+ s.param5 AS heap_tuples_written,
+ s.param6 AS heap_blks_total,
+ s.param7 AS heap_blks_scanned,
+ s.param8 AS index_rebuild_count
+ FROM (pg_stat_get_progress_info('REPACK'::text) s(pid, datid, relid, pa=
ram1, param2, param3, param4, param5, param6, param7, param8, param9, param=
10, param11, param12, param13, param14, param15, param16, param17, param18,=
param19, param20)
+ LEFT JOIN pg_database d ON ((s.datid =3D d.oid)));
pg_stat_progress_vacuum| SELECT s.pid,
s.datid,
d.datname,
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluste=
r.sql
index b7115f86104..c976823a3cb 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -76,7 +76,6 @@ INSERT INTO clstr_tst (b, c) VALUES (1111, 'this should f=
ail');
SELECT conname FROM pg_constraint WHERE conrelid =3D 'clstr_tst'::regclass
ORDER BY 1;
=20
-
SELECT relname, relkind,
EXISTS(SELECT 1 FROM pg_class WHERE oid =3D c.reltoastrelid) AS hastoa=
st
FROM pg_class c WHERE relname LIKE 'clstr_tst%' ORDER BY relname;
@@ -229,6 +228,24 @@ SELECT relname, old.level, old.relkind, old.relfilenod=
e =3D new.relfilenode FROM o
CLUSTER clstrpart;
ALTER TABLE clstrpart SET WITHOUT CLUSTER;
ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+
+-- Check that REPACK sets new relfilenodes: it should process exactly the =
same
+-- tables as CLUSTER did.
+DROP TABLE old_cluster_info;
+DROP TABLE new_cluster_info;
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, =
relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class=
c ON c.oid=3Dtree.relid ;
+REPACK clstrpart USING INDEX clstrpart_idx;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, =
relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class=
c ON c.oid=3Dtree.relid ;
+SELECT relname, old.level, old.relkind, old.relfilenode =3D new.relfilenod=
e FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname)=
ORDER BY relname COLLATE "C";
+
+-- And finally the same for REPACK w/o index.
+DROP TABLE old_cluster_info;
+DROP TABLE new_cluster_info;
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, =
relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class=
c ON c.oid=3Dtree.relid ;
+REPACK clstrpart;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, =
relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class=
c ON c.oid=3Dtree.relid ;
+SELECT relname, old.level, old.relkind, old.relfilenode =3D new.relfilenod=
e FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname)=
ORDER BY relname COLLATE "C";
+
DROP TABLE clstrpart;
=20
-- Ownership of partitions is checked
@@ -313,6 +330,57 @@ EXPLAIN (COSTS OFF) SELECT * FROM clstr_expression WHE=
RE -a =3D -3 ORDER BY -a, b;
SELECT * FROM clstr_expression WHERE -a =3D -3 ORDER BY -a, b;
COMMIT;
=20
+----------------------------------------------------------------------
+--
+-- REPACK
+--
+----------------------------------------------------------------------
+
+-- REPACK handles individual tables identically to CLUSTER, but it's worth
+-- checking if it handles table hierarchies identically as well.
+REPACK clstr_tst USING INDEX clstr_tst_c;
+
+-- Verify that inheritance link still works
+INSERT INTO clstr_tst_inh VALUES (0, 100, 'in child table 2');
+SELECT a,b,c,substring(d for 30), length(d) from clstr_tst;
+
+-- Verify that foreign key link still works
+INSERT INTO clstr_tst (b, c) VALUES (1111, 'this should fail');
+
+SELECT conname FROM pg_constraint WHERE conrelid =3D 'clstr_tst'::regclass
+ORDER BY 1;
+
+-- Verify partial analyze works
+REPACK (ANALYZE) clstr_tst (a);
+REPACK (ANALYZE) clstr_tst;
+REPACK (VERBOSE) clstr_tst (a);
+
+-- REPACK w/o argument performs no ordering, so we can only check which ta=
bles
+-- have the relfilenode changed.
+RESET SESSION AUTHORIZATION;
+CREATE TEMP TABLE relnodes_old AS
+(SELECT relname, relfilenode
+FROM pg_class
+WHERE relname IN ('clstr_1', 'clstr_2', 'clstr_3'));
+
+SET SESSION AUTHORIZATION regress_clstr_user;
+SET client_min_messages =3D ERROR; -- order of "skipping" warnings may va=
ry
+REPACK;
+RESET client_min_messages;
+
+RESET SESSION AUTHORIZATION;
+CREATE TEMP TABLE relnodes_new AS
+(SELECT relname, relfilenode
+FROM pg_class
+WHERE relname IN ('clstr_1', 'clstr_2', 'clstr_3'));
+
+-- Do the actual comparison. Unlike CLUSTER, clstr_3 should have been
+-- processed because there is nothing like clustering index here.
+SELECT o.relname FROM relnodes_old o
+JOIN relnodes_new n ON o.relname =3D n.relname
+WHERE o.relfilenode <> n.relfilenode
+ORDER BY o.relname;
+
-- clean up
DROP TABLE clustertest;
DROP TABLE clstr_1;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs=
.list
index 241945734ec..5774e5d4cc9 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2579,6 +2579,8 @@ ReorderBufferTupleCidEnt
ReorderBufferTupleCidKey
ReorderBufferUpdateProgressTxnCB
ReorderTuple
+RepackCommand
+RepackStmt
ReparameterizeForeignPathByChild_function
ReplOriginId
ReplOriginXactState
--=20
2.47.3
--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
filename=v34-0002-Refactor-index_concurrently_create_copy-for-use-with.patch
^ permalink raw reply [nested|flat] 30+ messages in thread
end of thread, other threads:[~2026-02-16 19:49 UTC | newest]
Thread overview: 30+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-03-04 01:21 Next commitfest app release is planned for March 18th Jelte Fennema-Nio <[email protected]>
2025-03-04 09:22 ` Peter Eisentraut <[email protected]>
2025-03-04 10:33 ` Jelte Fennema-Nio <[email protected]>
2025-03-04 16:15 ` Peter Eisentraut <[email protected]>
2025-03-04 20:37 ` Jelte Fennema-Nio <[email protected]>
2025-03-06 16:11 ` Peter Eisentraut <[email protected]>
2025-03-06 17:08 ` Tom Lane <[email protected]>
2025-03-06 17:27 ` Jelte Fennema-Nio <[email protected]>
2025-03-04 10:34 ` Álvaro Herrera <[email protected]>
2025-03-04 12:35 ` Amit Kapila <[email protected]>
2025-03-04 13:30 ` Jelte Fennema-Nio <[email protected]>
2025-03-04 21:01 ` Andreas Karlsson <[email protected]>
2025-03-04 23:14 ` Jelte Fennema-Nio <[email protected]>
2025-03-05 00:24 ` Andreas Karlsson <[email protected]>
2025-03-04 14:26 ` Robert Haas <[email protected]>
2025-03-04 20:42 ` Jelte Fennema-Nio <[email protected]>
2025-03-04 16:31 ` Peter Geoghegan <[email protected]>
2025-03-04 17:02 ` Jelte Fennema-Nio <[email protected]>
2025-03-04 17:25 ` Peter Geoghegan <[email protected]>
2025-03-04 20:06 ` Jelte Fennema-Nio <[email protected]>
2025-03-04 20:50 ` Robert Haas <[email protected]>
2025-03-17 22:11 ` Jelte Fennema-Nio <[email protected]>
2025-03-21 16:41 ` Peter Eisentraut <[email protected]>
2025-03-21 17:53 ` Tom Lane <[email protected]>
2025-03-22 11:15 ` Jelte Fennema-Nio <[email protected]>
2025-03-22 14:31 ` Peter Geoghegan <[email protected]>
2025-03-22 14:49 ` Tom Lane <[email protected]>
2025-03-22 15:05 ` Peter Geoghegan <[email protected]>
2025-03-24 09:55 ` Peter Eisentraut <[email protected]>
2026-02-16 19:49 [PATCH 1/5] Add REPACK command Antonin Houska <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox