pqxx::connection_base does not have a virtual DTOR

[kalman5]

It looks like the pqxx::connection_base doesn't not have the virtual DTOR

[jtv]

That is correct. There are no virtual functions in this class, so there is no need for a virtual destructor.

[kalman5]

I was able to do:
std::unique_ptr<pqxx::connection_base> myConnection = foo();
and you can imagine that when myConnection went out of scope the connection to the database was not discarded indeed the close() is in ~basic_connection();
If you don't wanna make that DTOR virtual then better move the close() from ~basic_connection to ~connection_base.

[egorpugin]

True.
Until pqxx::connection_base isn't virtual (having virtual members/destructor), information about the true type under pointer is lost.
Consider these examples:

struct s1 { ~s1(){} };
struct s2 : s1 { ~s2() { /* not executed */ } };
int main()
{
    std::unique_ptr<s1> p = std::make_unique<s2>();
}

struct s1 { virtual ~s1(){} };
struct s2 : s1 { ~s2() { /* executed */ } };
int main()
{
    std::unique_ptr<s1> p = std::make_unique<s2>();
}

So, the rule of thumb: if you intend pointers usage like @kalman5 mentioned, or you have any virtual methods in child classes, you must always make destructor virtual in the very base class of hierarchy.

[kalman5]

Fact is that most of methods take a pqxx:connection_base for reference so it seems valid to have pointers to pqxx::connection_base around, but deleting a connection trough a pqxx::connection_base pointer leaks the connection. I'd say fine that pqxx::connection_base doesn't have a virtual table but then in his destructor we need to call the close().

[jtv]

I suppose I always assumed that whoever created the connection would also ensure that it was closed, with the full type information at hand. If nothing else, the advent of move semantics may have upset that calculation.

Something else that puzzles me now, decades later, is that I don't see why connectionpolicy should have a vtable.

[kalman5]

You can remove the vtable entirely from connectionpolicy, indeed you are using a specialization of it inside basic_connection trough a pointer to the base class neither trough a reference.

[jtv]

Ah, no — connection_base::m_policy is a reference to connectionpolicy. So that needs the vtable.

I think some design considerations are starting to come back to me... The close() is done in the basic_connection destructor so that it would have the complete static type information, and a complete basic_connection object. Without that, it becomes hard to guarantee that you'll still have a working connectionpolicy when the connection_base destructor gets run.

It's all pretty brittle, but ISTM changing this without breaking things could require some invasive surgery, and not necessarily take us to a better design. Perhaps the best thing I can do right now is document it properly!

[jtv]

Looks like we can solve this along with #58 and #113. I'll mark this one a duplicate of #58, and when we make this change, we'll call 7.0.

[jtv]

Not released yet, but in master now: we're in the run-up to 7.0, where the connection hierarchy is now just a single class. No vtable required!

If you don't mind I'll close this ticket now, so it's off the to-do list. Please try the latest development versions!