Re: Fault injection framework

On Tue, Aug 27, 2019 at 12:35 PM Michael Paquier <michael(at)paquier(dot)xyz>
wrote:
>
> You may want to double-check whitespaces in your patch set, and 0002
> does not apply because of conflicts in isolationtester.h (my fault!).
>

I've rebased the patch set against the latest master and tried to resolve
whitespace issues. Apologies for the whitespace conflicts, I tried
resolving them but there is some trailing whitespace in the answer file of
the regress test in v1-0001 that cannot be removed, else the test will fail.

> 0002 is an independent feature, so I would keep it out of the fault
> framework for integration. There has been a argument from Alvaro
> more convincing than mine about the use of a separate keyword, hence
> removing a dependency with steps:
>
https://www.postgresql.org/message-id/20190823153825.GA11405@alvherre.pgsql
>

That is a valid point, thank you Alvaro for the feedback. I've changed
0002 so that a step within a permutation can be declared as blocking,
revised patch set is attached.

> It would be good also to have a test case which exercises it, without
> the need of the fault framework or its dedicated schedule.
>

It is for this reason that I have not separated patch 0002 out from
faultinjector patch set because the test to demonstrate the blocking
feature uses faults. I need to give more thought to find a test having a
session that needs to block for reasons other than locking. Any pointers
will be very helpful.

>
> My first impressions about this patch is that it is very intrusive.
> Could you explain the purpose of am_faultinjector? That's a specific
> connection string parameter which can be used similarly to replication
> for WAL senders? Couldn't there be an equivalent with a SUSET GUC?

Thank you for the review. Admittedly, the patch set doesn't include a test
to demonstrate am_faultinjector. That is used when a fault needs to be
injected into a remote server, say a standby. And that standby may be
accepting connections or not, depending on if it's operating in hot-standby
mode. Therefore, the am_faultinjector and the connection parameter is used
to identify fault injection requests and allow those to be handled even
when normal user connections are not allowed. Also, such connections do
not need to be associated with a database, they simply need to set the
fault in the shared memory hash table. In that sense, fault injection
connections are treated similar to replication connections.

I was looking into tests under src/test/recovery/t/. Let me write a test
to demonstrate what I'm trying to explain above.

> It may be interesting to see which parts of this framework could be
> moved into an extension loaded with shared_preload_libraries, one
> thing being the shared memory initialization part. At the end it
> would be interesting to not have a dependency with a compile-time
> flag.

Patch 0001 includes an extension that provides a SQL UDF as a wrapper over
the fault injection interface in backend. Moving the backend part of the
patch also into an extension seems difficult to me. Getting rid of the
compile time dependency is a strong enough advantage to spend more efforts
on this.

>
> Things like exec_fault_injector_command() need to be more documented.
> It is hard to guess what it is being used for.

Added a comment to explain things a bit. Hope that helps. And as
mentioned above, I'm working on a test case to demonstrate this feature.

Asim