On Fri, Jul 31, 2020 at 2:51 PM James Coleman <jtc331@gmail.com> wrote:
>
> On Thu, Jul 16, 2020 at 7:34 PM David Johnston
> <david.g.johnston@gmail.com> wrote:
> >
> > The following review has been posted through the commitfest application:
> > make installcheck-world: not tested
> > Implements feature: not tested
> > Spec compliant: not tested
> > Documentation: tested, passed
> >
> > James,
> >
> > I'm on board with the point of pointing out explicitly the "concurrent index builds on multiple tables at the same
timewill not return on any one table until all have completed", with back-patching. I do not believe the new paragraph
isnecessary though. I'd suggest trying to weave it into the existing paragraph ending "Even then, however, the index
maynot be immediately usable for queries: in the worst case, it cannot be used as long as transactions exist that
predatethe start of the index build." Adding "Notably, " in front of the existing sentence fragment above and tacking
itonto the end probably suffices.
>
> I'm not sure "the index may not be immediately usable for queries" is
> really accurate/sufficient: it seems to imply the CREATE INDEX has
> returned but for some reason the index isn't yet valid. The issue I'm
> trying to describe here is that the CREATE INDEX query itself will not
> return until all preceding queries have completed *including*
> concurrent index creations on unrelated tables.
>
> > I don't actually don't whether this is true behavior though. Is it something our tests do, or could, demonstrate?
>
> It'd take tests that exercise parallelism, but it's pretty simple to
> demonstrate (but you do have to catch the first index build in a scan
> phase, so you either need lots of data or a hack). Here's an example
> that uses a bit of a hack to simulate a slow scan phase:
>
> Setup:
> create table items(i int);
> create table others(i int);
> create function slow_expr() returns text as $$ select pg_sleep(15);
> select '5'; $$ language sql immutable;
> insert into items(i) values (1), (2);
> insert into others(i) values (1), (2);
>
> Then the following in order:
> 1. In session A: create index concurrently on items((i::text || slow_expr()));
> 2. In session B (at the same time): create index concurrently on others(i);
>
> You'll notice that the 2nd command, which should be practically
> instantaneous, waits on the first ~30s scan phase of (1) before it
> returns. The same is true if after (2) completes you immediately run
> it again -- it waits on the second ~30s scan phase of (1).
>
> That does reveal a bit of complexity though that that the current
> patch doesn't address, which is that this can be phase dependent (and
> that complexity gets a lot more non-obvious when there's real live
> activity (particularly long-running transactions) in the system as
> well.
>
> I've attached a new patch series with two items:
> 1. A simpler (and I believe more correct) doc changes for "cic blocks
> cic on other tables".
> 2. A patch to document that all index builds can prevent tuples from
> being vacuumed away on other tables.
>
> If it's preferable we could commit the first and discuss the second
> separately, but since that limitation was also discussed up-thread, I
> decided to include them both here for now.
Álvaro's patch confused the current state of this thread, so I'm
reattaching (rebased) v2 as v3.
James
